Page MenuHomePhabricator

[qa] Test that mempool reverts to pre-phonon policies
AbandonedPublic

Authored by dagurval on Feb 10 2020, 21:00.

Details

Reviewers
deadalnix
markblundeberg
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

This makes sure that mempool reverts to pre-phonon policies in case of a chain rollback.

Depends on D5244

Test Plan

Test added.
./test_runner.py && ./test_runner.py --with-phonon

Diff Detail

Repository
rABC Bitcoin ABC
Branch
trim-mempool-03
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9399
Build 16728: Default Diff Build & Tests
Build 16727: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Feb 10 2020, 21:00

Snippet of first build failure:

[21:01:40] :	 [Step 1/1] [287/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[21:01:40] :	 [Step 1/1] [288/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[21:01:40] :	 [Step 1/1] [289/404] Building CXX object src/CMakeFiles/server.dir/rpc/mining.cpp.o
[21:01:41] :	 [Step 1/1] [290/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[21:01:41] :	 [Step 1/1] [291/404] Building Java objects for secp256k1-jni-test-jar.jar
[21:01:41] :	 [Step 1/1] [292/404] Generating CMakeFiles/secp256k1-jni-test-jar.dir/java_class_filelist
[21:01:42] :	 [Step 1/1] [293/404] Creating Java archive secp256k1-jni-test.jar
[21:01:42] :	 [Step 1/1] [294/404] Building CXX object src/CMakeFiles/script.dir/script/interpreter.cpp.o
[21:01:42] :	 [Step 1/1] [295/404] Linking CXX static library src/libscript.a
[21:01:42] :	 [Step 1/1] [296/404] Linking CXX static library src/libbitcoinconsensus.a
[21:01:43] :	 [Step 1/1] [297/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/finaltx.cpp.o
[21:01:43] :	 [Step 1/1] [298/404] Linking CXX shared library src/libbitcoinconsensus.so.0.0.0
[21:01:43] :	 [Step 1/1] [299/404] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[21:01:45] :	 [Step 1/1] [300/404] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[21:01:46] :	 [Step 1/1] [301/404] Linking CXX executable src/bitcoin-tx
[21:01:47] :	 [Step 1/1] [302/404] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[21:01:47] :	 [Step 1/1] [303/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[21:01:47] :	 [Step 1/1] [304/404] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[21:01:48] :	 [Step 1/1] [305/404] Building CXX object src/seeder/CMakeFiles/seeder-netprocessing.dir/bitcoin.cpp.o
[21:01:48] :	 [Step 1/1] [306/404] Building CXX object src/seeder/CMakeFiles/seeder-netprocessing.dir/db.cpp.o
[21:01:48] :	 [Step 1/1] [307/404] Building CXX object src/seeder/CMakeFiles/seeder-netprocessing.dir/dns.cpp.o
[21:01:48] :	 [Step 1/1] [308/404] Linking CXX static library src/seeder/libseeder-netprocessing.a
[21:01:48] :	 [Step 1/1] [309/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[21:01:48] :	 [Step 1/1] [310/404] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[21:01:48] :	 [Step 1/1] [311/404] Building CXX object src/CMakeFiles/server.dir/txmempool.cpp.o
[21:01:48] :	 [Step 1/1] [312/404] Linking CXX executable src/seeder/bitcoin-seeder
[21:01:49] :	 [Step 1/1] [313/404] Building CXX object src/CMakeFiles/server.dir/init.cpp.o
[21:01:51] :	 [Step 1/1] [314/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/init.cpp.o
[21:01:52] :	 [Step 1/1] [315/404] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
[21:01:52] :	 [Step 1/1] [316/404] Building CXX object src/CMakeFiles/server.dir/validationinterface.cpp.o
[21:01:54] :	 [Step 1/1] [317/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[21:01:56] :	 [Step 1/1] [318/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[21:01:56] :	 [Step 1/1] [319/404] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[21:01:57] :	 [Step 1/1] [320/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[21:02:02] :	 [Step 1/1] [321/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[21:02:04] :	 [Step 1/1] [322/404] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[21:02:04] :	 [Step 1/1] [323/404] Linking CXX static library src/wallet/libwallet.a
[21:02:05] :	 [Step 1/1] [324/404] Linking CXX static library src/libserver.a
[21:02:07] :	 [Step 1/1] [325/404] Linking CXX executable src/bitcoind
[21:02:07] :	 [Step 1/1] FAILED: src/bitcoind 
[21:02:07] :	 [Step 1/1] : && /usr/bin/c++  -g -O2  -Wl,-z,relro -Wl,-z,now -pie src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o  -o src/bitcoind  src/libserver.a src/wallet/libwallet.a src/libserver.a src/wallet/libwallet.a src/libbitcoinconsensus.a src/leveldb/libleveldb.a src/leveldb/libleveldb-sse4.2.a src/leveldb/libmemenv.a /usr/lib/x86_64-linux-gnu/libminiupnpc.so src/zmq/libzmq.a /usr/lib/x86_64-linux-gnu/libzmq.so /usr/lib/x86_64-linux-gnu/libevent.so /usr/lib/x86_64-linux-gnu/libevent_pthreads.so src/libscript.a src/libcommon.a src/libutil.a src/crypto/libcrypto.a /usr/lib/x86_64-linux-gnu/libcrypto.so src/crypto/libcrypto_sse4.1.a src/crypto/libcrypto_avx2.a src/crypto/libcrypto_shani.a /usr/lib/x86_64-linux-gnu/libboost_filesystem.so /usr/lib/x86_64-linux-gnu/libboost_thread.so /usr/lib/x86_64-linux-gnu/libboost_chrono.so /usr/lib/x86_64-linux-gnu/libboost_system.so /usr/lib/x86_64-linux-gnu/libboost_date_time.so /usr/lib/x86_64-linux-gnu/libboost_atomic.so -pthread src/secp256k1/libsecp256k1.a src/univalue/libunivalue.a /usr/lib/x86_64-linux-gnu/libdb_cxx.so && :
[21:02:07] :	 [Step 1/1] /usr/bin/ld: src/libserver.a(validation.cpp.o): in function `AcceptToMemoryPoolWorker(Config const&, CTxMemPool&, CValidationState&, std::shared_ptr<CTransaction const> const&, bool*, long, bool, Amount, std::vector<COutPoint, std::allocator<COutPoint> >&, bool) [clone .constprop.1798]':
[21:02:07] :	 [Step 1/1] /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:747: undefined reference to `GetDefaultAncestorLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] /usr/bin/ld: /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:753: undefined reference to `GetDefaultDecendantLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] /usr/bin/ld: src/libserver.a(validation.cpp.o): in function `CChainState::DisconnectTip(Config const&, CValidationState&, DisconnectedBlockTransactions*)':
[21:02:07] :	 [Step 1/1] /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:2288: undefined reference to `GetDefaultAncestorLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] /usr/bin/ld: /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:2289: undefined reference to `GetDefaultAncestorLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] /usr/bin/ld: /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:2292: undefined reference to `GetDefaultDecendantLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] /usr/bin/ld: /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/../src/validation.cpp:2293: undefined reference to `GetDefaultDecendantLimit(Consensus::Params const&, CBlockIndex const*)'
[21:02:07] :	 [Step 1/1] collect2: error: ld returned 1 exit status
[21:02:07] :	 [Step 1/1] [326/404] Automatic MOC for target bitcoin-qt-base
[21:02:07] :	 [Step 1/1] ninja: build stopped: subcommand failed.
[21:02:07] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[21:02:07]W:	 [Step 1/1] ++ print_sanitizers_log
[21:02:07]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[21:02:07]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[21:02:07]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[21:02:07]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[21:02:07]W:	 [Step 1/1] Process exited with code 1
[21:02:07]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)

Marking as planned changes due to issues with parent diff

Nit: Filename "abc-phonos-disconnectpool.py" should change phonos -> phonon

Rebase, rename test, some test cleanup

deadalnix requested changes to this revision.Feb 13 2020, 01:38
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/validation.cpp
2293 ↗(On Diff #16277)

I don't think this is necessary because the flags change anyways when phonon is enabled. I expect that the test will pass anyways even if you remove this.

This revision now requires changes to proceed.Feb 13 2020, 01:38

Remove redundant changes in validation.cpp

dagurval retitled this revision from Remove longer ancestor/descendants chain in case of reorg to [qa] Test that mempool reverts to pre-phonon policies.Feb 13 2020, 14:15
dagurval edited the summary of this revision. (Show Details)
markblundeberg added a subscriber: markblundeberg.

Yes this works because the mempool is being reprocessed now due to another upgrade, as the reversebytes thing D5130 (==D5283) is landed already. 👍
(that's not bad btw, I rely on the same in my now-landed D5224 tests)

test/functional/abc-phonon-mempoolpolicy.py
22 ↗(On Diff #16353)

Phonon, also just use # for comments and reserve """ for docstrings. As far as I'm aware python doesn't have per-variable docstrings.

dagurval added inline comments.
test/functional/abc-phonon-mempoolpolicy.py
22 ↗(On Diff #16353)

I thought it was standard because sphinx picks up """ docstring for variables when they're added below them. I think you're right though, changing to #

jasonbcox requested changes to this revision.Feb 14 2020, 18:11
jasonbcox added a subscriber: jasonbcox.

Just pointing out that not including this as part of D5244 makes getting this feature in more difficult, since both patches will need to be reviewed green by potentially different sets of people (and preferably landed very soon one after the other).

test/functional/abc-phonon-mempoolpolicy.py
41 ↗(On Diff #16377)

This function looks fragile. Check how it's done in abc-op-reversebytes-activation.py:203

58 ↗(On Diff #16377)

Make 0.0001 a string, otherwise it's possible to run into this problem: https://stackoverflow.com/questions/32053647/comparing-python-decimals-created-from-float-and-string

95 ↗(On Diff #16377)

This is good, but a similar test needs to done to make sure that long-chains get trimmed even if they were mined.

This revision now requires changes to proceed.Feb 14 2020, 18:11

I'll merge the test into D5244 as advised by @jasonbcox