Page MenuHomePhabricator
Differential D5245

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

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 9495
Build 16903: Default Diff Build & Tests
Build 16902: arc lint + arc unit

Event Timeline

dagurval created this revision.Feb 10 2020, 21:00
Owners added a reviewer: Restricted Owners Package.Feb 10 2020, 21:00
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 10 2020, 21:00
teamcity added a comment.Feb 10 2020, 21:00

Build Bitcoin-ABC / Diffs / Diff Testing started.

dagurval edited the summary of this revision. (Show Details)Feb 10 2020, 21:01
dagurval added a parent revision: D5244: Enable new ancestor/descendants chains limit at fork.
teamcity added a comment.Feb 10 2020, 21:02
Build Bitcoin-ABC / Diffs / Diff Testing failed.

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)
Harbormaster failed remote builds in B9399: Diff 16234!Feb 10 2020, 21:02
dagurval planned changes to this revision.Feb 10 2020, 22:33

Marking as planned changes due to issues with parent diff

Mengerian added a subscriber: Mengerian.Feb 11 2020, 04:13

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

dagurval updated this revision to Diff 16277.Feb 11 2020, 13:31

Rebase, rename test, some test cleanup

teamcity added a comment.Feb 11 2020, 13:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 11 2020, 13:41

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9433: Diff 16277.Feb 11 2020, 13:41
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
dagurval planned changes to this revision.Feb 13 2020, 09:35
dagurval updated this revision to Diff 16353.Feb 13 2020, 14:14

Remove redundant changes in validation.cpp

teamcity added a comment.Feb 13 2020, 14:14

Build Bitcoin-ABC / Diffs / Diff Testing started.

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)
teamcity added a comment.Feb 13 2020, 14:23

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9480: Diff 16353.Feb 13 2020, 14:23
markblundeberg accepted this revision as: markblundeberg.Feb 14 2020, 02:22
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 planned changes to this revision.Feb 14 2020, 07:12
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 #

dagurval updated this revision to Diff 16377.Feb 14 2020, 07:14

typo, docstring fix

teamcity added a comment.Feb 14 2020, 07:14

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Feb 14 2020, 07:24

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9495: Diff 16377.Feb 14 2020, 07:24
dagurval mentioned this in D5244: Enable new ancestor/descendants chains limit at fork.Feb 14 2020, 16:23
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

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

58

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

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
dagurval abandoned this revision.Feb 14 2020, 19:54

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

Revision Contents
Changeset List

PathSize
test/
functional/
99 lines

Diff 16377

View Options

test/functional/abc-phonon-mempoolpolicy.py

Loading...