Page MenuHomePhabricator
Differential D13491

[avalanche] Sort txs to poll by modified fee rate
ClosedPublic
Actions

Authored by Fabien on Mar 28 2023, 20:21.

Details

Reviewers
PiRK
sdulfari
Group Reviewers
Restricted Project
Commits
rABC223c43df3eb5: [avalanche] Sort txs to poll by modified fee rate
Summary

Then fallback to entry id in case of equality. Also manage a sort by txid if the txs are not in the mempool (which is currently unsupported by avalanche).

Test Plan
ninja all check-all

Run the TSAN build.

Diff Detail

Event Timeline

Fabien created this revision.Mar 28 2023, 20:21
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 28 2023, 20:21
Fabien requested review of this revision.Mar 28 2023, 20:21
Harbormaster completed remote builds in B22796: Diff 39004.Mar 28 2023, 20:28
teamcity added a comment.Mar 28 2023, 20:28
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

wallet_keypool.py                         | ○ Skipped | 0 s
wallet_keypool_topup.py                   | ○ Skipped | 0 s
wallet_keypool_topup.py --descriptors     | ○ Skipped | 0 s
wallet_labels.py                          | ○ Skipped | 0 s
wallet_labels.py --descriptors            | ○ Skipped | 0 s
wallet_listreceivedby.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py --descriptors    | ○ Skipped | 0 s
wallet_listtransactions.py                | ○ Skipped | 0 s
wallet_listtransactions.py --descriptors  | ○ Skipped | 0 s
wallet_multiwallet.py --usecli            | ○ Skipped | 0 s
wallet_reorgsrestore.py                   | ○ Skipped | 0 s
wallet_resendwallettransactions.py        | ○ Skipped | 0 s
wallet_send.py                            | ○ Skipped | 0 s
wallet_startup.py                         | ○ Skipped | 0 s
wallet_timelock.py                        | ○ Skipped | 0 s
wallet_txn_clone.py                       | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock           | ○ Skipped | 0 s
wallet_txn_doublespend.py                 | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock     | ○ Skipped | 0 s
wallet_watchonly.py                       | ○ Skipped | 0 s
wallet_watchonly.py --usecli              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 759 s (accumulated) 
Runtime: 152 s

[29/447] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.016s

OK
[30/447] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[118/447] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:540:22: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  540 | BOOST_AUTO_TEST_CASE(script_build) {
      |                      ^~~~~~~~~~~~
[176/447] Running seeder test suite
PASSED: seeder test suite
[182/447] Running pow test suite
PASSED: pow test suite
[188/447] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[425/447] Running bitcoin test suite
PASSED: bitcoin test suite
[442/447] avalanche: testing processor_tests
FAILED: src/avalanche/test/CMakeFiles/check-avalanche-processor_tests 
cd /work/abc-ci-builds/build-without-wallet/src/avalanche/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-without-wallet/test/log/avalanche-processor_tests.log /work/abc-ci-builds/build-without-wallet/src/avalanche/test/test-avalanche --run_test=processor_tests --logger=HRF,message:JUNIT,message,avalanche-processor_tests.xml --catch_system_errors=no
Segmentation fault (core dumped)
[444/447] Running utility command for check-avalanche-peermanager_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
teamcity added a comment.Mar 28 2023, 20:35
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

wallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
wallet_backup.py                          | ✓ Passed  | 20 s
wallet_balance.py                         | ✓ Passed  | 6 s
wallet_balance.py --descriptors           | ✓ Passed  | 6 s
wallet_basic.py                           | ✓ Passed  | 13 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 5 s
wallet_createwallet.py                    | ✓ Passed  | 2 s
wallet_createwallet.py --descriptors      | ✓ Passed  | 2 s
wallet_createwallet.py --usecli           | ✓ Passed  | 3 s
wallet_descriptor.py                      | ✓ Passed  | 6 s
wallet_disable.py                         | ✓ Passed  | 0 s
wallet_dump.py                            | ✓ Passed  | 4 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 11 s
wallet_hd.py                              | ✓ Passed  | 7 s
wallet_hd.py --descriptors                | ✓ Passed  | 6 s
wallet_import_rescan.py                   | ✓ Passed  | 9 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 5 s
wallet_importmulti.py                     | ✓ Passed  | 3 s
wallet_importprunedfunds.py               | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
wallet_keypool.py                         | ✓ Passed  | 2 s
wallet_keypool_topup.py                   | ✓ Passed  | 3 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 5 s
wallet_labels.py                          | ✓ Passed  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 1 s
wallet_listreceivedby.py                  | ✓ Passed  | 5 s
wallet_listsinceblock.py                  | ✓ Passed  | 6 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 7 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 4 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 9 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 5 s
wallet_send.py                            | ✓ Passed  | 6 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_timelock.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py                       | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s
chronik_block.py                          | ○ Skipped | 0 s
chronik_disallow_prune.py                 | ○ Skipped | 0 s
chronik_resync.py                         | ○ Skipped | 0 s
chronik_serve.py                          | ○ Skipped | 0 s
chronik_tx.py                             | ○ Skipped | 0 s
interface_usdt_net.py                     | ○ Skipped | 0 s
interface_usdt_utxocache.py               | ○ Skipped | 0 s
interface_usdt_validation.py              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 1225 s (accumulated) 
Runtime: 245 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
Fabien planned changes to this revision.Mar 28 2023, 21:02

I need to investigate the failure, apparently it only happens with GCC

Fabien updated this revision to Diff 39011.EditedMar 29 2023, 08:00

"Fix" the GCC failure.
I managed to reproduce the issue locally, and this only occurs on unit tests with GCC and no debug build.
From digging it appears that the unit test cases could run in parallel and causing the lock for an undetermined reason (the locks are supposed to be different instances so they should not conflict).
Fix the issue by squashing the cases and also run TSAN (added to test plan) for good measure.

Worth noting that this is impossible to produce outside of unit tests as there is a single vote map instance.

Fabien edited the test plan for this revision. (Show Details)Mar 29 2023, 08:02
Harbormaster completed remote builds in B22801: Diff 39011.Mar 29 2023, 08:03
teamcity added a comment.Mar 29 2023, 08:03
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.

Tail of the build log:

[411/486] bitcoin: testing inv_tests
[412/486] Running utility command for check-bitcoin-fs_tests
[413/486] bitcoin: testing checkpoints_tests
[414/486] Running utility command for check-bitcoin-inv_tests
[415/486] Running utility command for check-bitcoin-checkpoints_tests
[416/486] bitcoin: testing sigencoding_tests
[417/486] Running utility command for check-bitcoin-sigencoding_tests
[418/486] bitcoin: testing script_tests
[419/486] bitcoin: testing coinstatsindex_tests
[420/486] Running utility command for check-bitcoin-script_tests
[421/486] Running utility command for check-bitcoin-coinstatsindex_tests
[422/486] bitcoin: testing scheduler_tests
[423/486] bitcoin: testing blockfilter_tests
[424/486] bitcoin: testing key_io_tests
[425/486] Running utility command for check-bitcoin-blockfilter_tests
[426/486] Running utility command for check-bitcoin-scheduler_tests
[427/486] Running utility command for check-bitcoin-key_io_tests
[428/486] bitcoin: testing scriptpubkeyman_tests
[429/486] bitcoin: testing txvalidationcache_tests
[430/486] Running utility command for check-bitcoin-scriptpubkeyman_tests
[431/486] Running utility command for check-bitcoin-txvalidationcache_tests
[432/486] bitcoin: testing cuckoocache_tests
[433/486] Running utility command for check-bitcoin-cuckoocache_tests
[434/486] bitcoin: testing walletdb_tests
[435/486] bitcoin: testing crypto_tests
[436/486] bitcoin: testing logging_tests
[437/486] Running utility command for check-bitcoin-walletdb_tests
[438/486] bitcoin: testing merkle_tests
[439/486] Running utility command for check-bitcoin-crypto_tests
[440/486] Running utility command for check-bitcoin-logging_tests
[441/486] Running utility command for check-bitcoin-merkle_tests
[442/486] bitcoin: testing psbt_wallet_tests
[443/486] Running utility command for check-bitcoin-psbt_wallet_tests
[444/486] bitcoin: testing denialofservice_tests
[445/486] Running utility command for check-bitcoin-denialofservice_tests
[446/486] bitcoin: testing init_tests
[447/486] Running utility command for check-bitcoin-init_tests
[448/486] bitcoin: testing rcu_tests
[449/486] Running utility command for check-bitcoin-rcu_tests
[450/486] bitcoin: testing wallet_crypto_tests
[451/486] Running utility command for check-bitcoin-wallet_crypto_tests
[452/486] bitcoin: testing txrequest_tests
[453/486] Running utility command for check-bitcoin-txrequest_tests
[454/486] bitcoin: testing blockcheck_tests
[455/486] Running utility command for check-bitcoin-blockcheck_tests
[456/486] bitcoin: testing wallet_tests
[457/486] Running utility command for check-bitcoin-wallet_tests
[458/486] bitcoin: testing coinselector_tests
[459/486] Running utility command for check-bitcoin-coinselector_tests
[460/486] bitcoin: testing transaction_tests
[461/486] Running utility command for check-bitcoin-transaction_tests
[462/486] bitcoin: testing coins_tests
[463/486] Running utility command for check-bitcoin-coins_tests
[464/486] Running bitcoin test suite
PASSED: bitcoin test suite
[465/486] secp256k1: testing secp256k1-tests
[466/486] Running secp256k1 test suite
PASSED: secp256k1 test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1
Fabien planned changes to this revision.Mar 29 2023, 08:06
teamcity added a comment.Mar 29 2023, 08:20
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

wallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
wallet_backup.py                          | ✓ Passed  | 18 s
wallet_balance.py                         | ✓ Passed  | 6 s
wallet_balance.py --descriptors           | ✓ Passed  | 6 s
wallet_basic.py                           | ✓ Passed  | 14 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 5 s
wallet_createwallet.py                    | ✓ Passed  | 2 s
wallet_createwallet.py --descriptors      | ✓ Passed  | 2 s
wallet_createwallet.py --usecli           | ✓ Passed  | 3 s
wallet_descriptor.py                      | ✓ Passed  | 6 s
wallet_disable.py                         | ✓ Passed  | 1 s
wallet_dump.py                            | ✓ Passed  | 4 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_groups.py                          | ✓ Passed  | 12 s
wallet_hd.py                              | ✓ Passed  | 5 s
wallet_hd.py --descriptors                | ✓ Passed  | 6 s
wallet_import_rescan.py                   | ✓ Passed  | 7 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 5 s
wallet_importmulti.py                     | ✓ Passed  | 2 s
wallet_importprunedfunds.py               | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 4 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 5 s
wallet_labels.py                          | ✓ Passed  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 1 s
wallet_listreceivedby.py                  | ✓ Passed  | 5 s
wallet_listsinceblock.py                  | ✓ Passed  | 5 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 6 s
wallet_listtransactions.py                | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 4 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 9 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 8 s
wallet_send.py                            | ✓ Passed  | 7 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_timelock.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s
chronik_block.py                          | ○ Skipped | 0 s
chronik_disallow_prune.py                 | ○ Skipped | 0 s
chronik_resync.py                         | ○ Skipped | 0 s
chronik_serve.py                          | ○ Skipped | 0 s
chronik_tx.py                             | ○ Skipped | 0 s
interface_usdt_net.py                     | ○ Skipped | 0 s
interface_usdt_utxocache.py               | ○ Skipped | 0 s
interface_usdt_validation.py              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 1236 s (accumulated) 
Runtime: 247 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
teamcity added a comment.Mar 29 2023, 08:23
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

wallet_import_with_label.py               | ○ Skipped | 0 s
wallet_importdescriptors.py               | ○ Skipped | 0 s
wallet_importmulti.py                     | ○ Skipped | 0 s
wallet_importprunedfunds.py               | ○ Skipped | 0 s
wallet_importprunedfunds.py --descriptors | ○ Skipped | 0 s
wallet_keypool.py                         | ○ Skipped | 0 s
wallet_keypool_topup.py                   | ○ Skipped | 0 s
wallet_keypool_topup.py --descriptors     | ○ Skipped | 0 s
wallet_labels.py                          | ○ Skipped | 0 s
wallet_labels.py --descriptors            | ○ Skipped | 0 s
wallet_listreceivedby.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py                  | ○ Skipped | 0 s
wallet_listsinceblock.py --descriptors    | ○ Skipped | 0 s
wallet_listtransactions.py                | ○ Skipped | 0 s
wallet_listtransactions.py --descriptors  | ○ Skipped | 0 s
wallet_multiwallet.py --usecli            | ○ Skipped | 0 s
wallet_reorgsrestore.py                   | ○ Skipped | 0 s
wallet_resendwallettransactions.py        | ○ Skipped | 0 s
wallet_send.py                            | ○ Skipped | 0 s
wallet_startup.py                         | ○ Skipped | 0 s
wallet_timelock.py                        | ○ Skipped | 0 s
wallet_txn_clone.py                       | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock           | ○ Skipped | 0 s
wallet_txn_doublespend.py                 | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock     | ○ Skipped | 0 s
wallet_watchonly.py                       | ○ Skipped | 0 s
wallet_watchonly.py --usecli              | ○ Skipped | 0 s

ALL                                       | ✓ Passed  | 766 s (accumulated) 
Runtime: 153 s

[30/447] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

OK
[31/447] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[126/447] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:540:22: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  540 | BOOST_AUTO_TEST_CASE(script_build) {
      |                      ^~~~~~~~~~~~
[181/447] Running seeder test suite
PASSED: seeder test suite
[186/447] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[405/447] Running pow test suite
PASSED: pow test suite
[425/447] Running bitcoin test suite
PASSED: bitcoin test suite
[443/447] Running utility command for check-avalanche-peermanager_tests
Build build-without-wallet timed out after 1200.0s
Fabien updated this revision to Diff 39012.Mar 29 2023, 09:06

Fix the shadowing warning, explicitely nullify the mempool

Harbormaster completed remote builds in B22802: Diff 39012.Mar 29 2023, 09:19
Fabien updated this revision to Diff 39039.Mar 30 2023, 07:59
  • Ensure mempool is nulled when using the default vote comparator constructor
  • Move the collection instead of copying in the RWCollection constructor
  • Make the voteRecord initialization use the explicit constructor when building the Processor
Harbormaster completed remote builds in B22821: Diff 39039.Mar 30 2023, 08:16
PiRK accepted this revision.Mar 30 2023, 13:26
This revision is now accepted and ready to land.Mar 30 2023, 13:26
sdulfari requested changes to this revision.Mar 30 2023, 18:29
sdulfari added a subscriber: sdulfari.
sdulfari added inline comments.
src/avalanche/test/processor_tests.cpp
2105 ↗(On Diff #39039)

This part of the test onward looks like it should be its own test. Other than a few txs with fees in the mempool, there does not seem to be anything else reused.

This revision now requires changes to proceed.Mar 30 2023, 18:29
Fabien updated this revision to Diff 39099.Mar 31 2023, 13:15

Cut the test in 2 cases

Fabien updated this revision to Diff 39100.Mar 31 2023, 13:27

Fix scopes

Harbormaster completed remote builds in B22869: Diff 39099.Mar 31 2023, 13:31
Harbormaster completed remote builds in B22870: Diff 39100.Mar 31 2023, 13:51
sdulfari accepted this revision.Mar 31 2023, 18:55
This revision is now accepted and ready to land.Mar 31 2023, 18:55
Closed by commit rABC223c43df3eb5: [avalanche] Sort txs to poll by modified fee rate (authored by Fabien). · Explain WhyApr 3 2023, 15:27
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC223c43df3eb5: [avalanche] Sort txs to poll by modified fee rate.
Fabien added a reverting change: D16637: Revert "[avalanche] Sort txs to poll by modified fee rate".Aug 13 2024, 14:10
Fabien added a reverting change: rABC77a9534f0e63: Revert "[avalanche] Sort txs to poll by modified fee rate".Aug 13 2024, 19:49

Revision Contents
Changeset List

PathSize
src/
avalanche/
40 lines
6 lines
test/
161 lines
1 line

Diff 39011

View Options

src/avalanche/processor.h

Loading...
View Options

src/avalanche/processor.cpp

Loading...
View Options

src/avalanche/test/processor_tests.cpp

Loading...
View Options

src/rwcollection.h

Loading...