Page MenuHomePhabricator

Return a set instead of a vector for the miner fund whitelist
ClosedPublic

Authored by sdulfari on Feb 1 2023, 22:09.

Details

Summary

This will make writing a robust implementation of GetMinerFundWhitelist easier
as we add another path for a policy whitelist.

Also took this opportunity to use auto where appropriate.

Test Plan
ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Branch
minerfund-set
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 21947
Build 43529: Build Diff
Build 43528: arc lint + arc unit

Event Timeline

sdulfari requested review of this revision.Feb 1 2023, 22:09

Tail of the build log:

[178/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[179/480] pow: testing aserti32d_tests
[180/480] Running utility command for check-pow-aserti32d_tests
[181/480] Running pow test suite
PASSED: pow test suite
[182/480] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validationinterface_tests.cpp.o
[183/480] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/versionbits_tests.cpp.o
[184/480] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[185/480] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[186/480] Linking CXX executable src/seeder/test/test-seeder
[187/480] seeder: testing message_writer_tests
[188/480] seeder: testing p2p_messaging_tests
[189/480] seeder: testing parse_name_tests
[190/480] seeder: testing options_tests
[191/480] Running utility command for check-seeder-message_writer_tests
[192/480] Running utility command for check-seeder-p2p_messaging_tests
[193/480] Running utility command for check-seeder-parse_name_tests
[194/480] seeder: testing write_name_tests
[195/480] Running utility command for check-seeder-options_tests
[196/480] Running utility command for check-seeder-write_name_tests
[197/480] Running seeder test suite
PASSED: seeder test suite
[198/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[199/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[200/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[201/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[202/480] Building CXX object src/avalanche/test/CMakeFiles/test-avalanche.dir/processor_tests.cpp.o
[203/480] Linking CXX executable src/avalanche/test/test-avalanche
[204/480] avalanche: testing init_tests
[205/480] Running utility command for check-avalanche-init_tests
[206/480] avalanche: testing delegation_tests
[207/480] Running utility command for check-avalanche-delegation_tests
[208/480] avalanche: testing proofcomparator_tests
[209/480] Running utility command for check-avalanche-proofcomparator_tests
[210/480] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[211/480] avalanche: testing proofpool_tests
[212/480] Running utility command for check-avalanche-proofpool_tests
[213/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[214/480] avalanche: testing proof_tests
[215/480] Running utility command for check-avalanche-proof_tests
[216/480] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[217/480] avalanche: testing compactproofs_tests
[218/480] Running utility command for check-avalanche-compactproofs_tests
[219/480] Linking CXX executable src/qt/test/test_bitcoin-qt
[220/480] bitcoin-qt: testing test_bitcoin-qt
[221/480] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[222/480] avalanche: testing processor_tests
[223/480] Running utility command for check-avalanche-processor_tests
[224/480] avalanche: testing voterecord_tests
[225/480] Running utility command for check-avalanche-voterecord_tests
[226/480] secp256k1: testing secp256k1-tests
[227/480] Running secp256k1 test suite
PASSED: secp256k1 test suite
[228/480] avalanche: testing peermanager_tests
[229/480] Running utility command for check-avalanche-peermanager_tests
[230/480] Running avalanche test suite
PASSED: avalanche test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

Tail of the build log:

[170/473] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/daa_tests.cpp.o
[171/473] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/options_tests.cpp.o
[172/473] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/write_name_tests.cpp.o
[173/473] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/parse_name_tests.cpp.o
[174/473] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/eda_tests.cpp.o
[175/473] Automatic MOC for target test_bitcoin-qt
[176/473] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/grasberg_tests.cpp.o
[177/473] avalanche: testing processor_tests
[178/473] Running utility command for check-avalanche-processor_tests
[179/473] Linking CXX executable src/pow/test/test-pow
[180/473] avalanche: testing peermanager_tests
[181/473] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[182/473] Test Bitcoin utilities...
[183/473] pow: testing daa_tests
[184/473] Running utility command for check-avalanche-peermanager_tests
[185/473] Running utility command for check-pow-daa_tests
[186/473] Running avalanche test suite
PASSED: avalanche test suite
[187/473] pow: testing eda_tests
[188/473] Running utility command for check-pow-eda_tests
[189/473] pow: testing grasberg_tests
[190/473] Running utility command for check-pow-grasberg_tests
[191/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[192/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[193/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[194/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[195/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[196/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[197/473] pow: testing aserti32d_tests
[198/473] Running utility command for check-pow-aserti32d_tests
[199/473] Running pow test suite
PASSED: pow test suite
[200/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[201/473] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[202/473] Linking CXX executable src/seeder/test/test-seeder
[203/473] seeder: testing message_writer_tests
[204/473] seeder: testing options_tests
[205/473] seeder: testing p2p_messaging_tests
[206/473] seeder: testing parse_name_tests
[207/473] Running utility command for check-seeder-message_writer_tests
[208/473] Running utility command for check-seeder-options_tests
[209/473] seeder: testing write_name_tests
[210/473] Running utility command for check-seeder-p2p_messaging_tests
[211/473] Running utility command for check-seeder-parse_name_tests
[212/473] Running utility command for check-seeder-write_name_tests
[213/473] Running seeder test suite
PASSED: seeder test suite
[214/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[215/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[216/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[217/473] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[218/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[219/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[220/473] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[221/473] Linking CXX executable src/qt/test/test_bitcoin-qt
[222/473] bitcoin-qt: testing test_bitcoin-qt
[223/473] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Tail of the build log:

                 from ../../src/test/minerfund_tests.cpp:5:
/usr/include/c++/9/bits/stl_iterator.h:886:5: note: candidate: ‘template<class _IteratorL, class _IteratorR, class _Container> bool __gnu_cxx::operator==(const __gnu_cxx::__normal_iterator<_IteratorL, _Container>&, const __gnu_cxx::__normal_iterator<_IteratorR, _Container>&)’
  886 |     operator==(const __normal_iterator<_IteratorL, _Container>& __lhs,
      |     ^~~~~~~~
/usr/include/c++/9/bits/stl_iterator.h:886:5: note:   template argument deduction/substitution failed:
In file included from /usr/include/boost/test/test_tools.hpp:45,
                 from /usr/include/boost/test/unit_test.hpp:18,
                 from ../../src/test/minerfund_tests.cpp:13:
../../src/test/minerfund_tests.cpp:46:17: note:   ‘std::set<boost::variant<CNoDestination, PKHash, ScriptHash> >’ is not derived from ‘const __gnu_cxx::__normal_iterator<_IteratorL, _Container>’
   46 |                 expectedMinerFund);
      |                 ^~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/9/bits/stl_algobase.h:67,
                 from /usr/include/c++/9/algorithm:61,
                 from ../../src/./prevector.h:8,
                 from ../../src/./serialize.h:10,
                 from ../../src/./consensus/amount.h:10,
                 from ../../src/./minerfund.h:8,
                 from ../../src/test/minerfund_tests.cpp:5:
/usr/include/c++/9/bits/stl_iterator.h:893:5: note: candidate: ‘template<class _Iterator, class _Container> bool __gnu_cxx::operator==(const __gnu_cxx::__normal_iterator<_Iterator, _Container>&, const __gnu_cxx::__normal_iterator<_Iterator, _Container>&)’
  893 |     operator==(const __normal_iterator<_Iterator, _Container>& __lhs,
      |     ^~~~~~~~
/usr/include/c++/9/bits/stl_iterator.h:893:5: note:   template argument deduction/substitution failed:
In file included from /usr/include/boost/test/test_tools.hpp:45,
                 from /usr/include/boost/test/unit_test.hpp:18,
                 from ../../src/test/minerfund_tests.cpp:13:
../../src/test/minerfund_tests.cpp:46:17: note:   ‘std::set<boost::variant<CNoDestination, PKHash, ScriptHash> >’ is not derived from ‘const __gnu_cxx::__normal_iterator<_Iterator, _Container>’
   46 |                 expectedMinerFund);
      |                 ^~~~~~~~~~~~~~~~~
In file included from /usr/include/x86_64-linux-gnu/c++/9/bits/c++allocator.h:33,
                 from /usr/include/c++/9/bits/allocator.h:46,
                 from /usr/include/c++/9/string:41,
                 from /usr/include/c++/9/stdexcept:39,
                 from /usr/include/c++/9/array:39,
                 from /usr/include/c++/9/tuple:39,
                 from /usr/include/c++/9/functional:54,
                 from /usr/include/c++/9/pstl/glue_algorithm_defs.h:13,
                 from /usr/include/c++/9/algorithm:71,
                 from ../../src/./prevector.h:8,
                 from ../../src/./serialize.h:10,
                 from ../../src/./consensus/amount.h:10,
                 from ../../src/./minerfund.h:8,
                 from ../../src/test/minerfund_tests.cpp:5:
/usr/include/c++/9/ext/new_allocator.h:166:2: note: candidate: ‘template<class _Up> bool __gnu_cxx::operator==(const __gnu_cxx::new_allocator<boost::variant<CNoDestination, PKHash, ScriptHash> >&, const __gnu_cxx::new_allocator<_Tp>&)’
  166 |  operator==(const new_allocator&, const new_allocator<_Up>&)
      |  ^~~~~~~~
/usr/include/c++/9/ext/new_allocator.h:166:2: note:   template argument deduction/substitution failed:
In file included from /usr/include/boost/test/test_tools.hpp:45,
                 from /usr/include/boost/test/unit_test.hpp:18,
                 from ../../src/test/minerfund_tests.cpp:13:
../../src/test/minerfund_tests.cpp:46:17: note:   ‘const std::vector<boost::variant<CNoDestination, PKHash, ScriptHash> >’ is not derived from ‘const __gnu_cxx::new_allocator<_Tp>’
   46 |                 expectedMinerFund);
      |                 ^~~~~~~~~~~~~~~~~
[198/441] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[203/441] Running pow test suite
PASSED: pow test suite
[208/441] Running avalanche test suite
PASSED: avalanche test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Tail of the build log:

wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 7 s
wallet_importmulti.py                     | ✓ Passed  | 5 s
wallet_importprunedfunds.py               | ✓ Passed  | 5 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 4 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 7 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 9 s
wallet_labels.py                          | ✓ Passed  | 2 s
wallet_labels.py --descriptors            | ✓ Passed  | 2 s
wallet_listreceivedby.py                  | ✓ Passed  | 9 s
wallet_listsinceblock.py                  | ✓ Passed  | 9 s
wallet_listsinceblock.py --descriptors    | ✓ Passed  | 11 s
wallet_listtransactions.py                | ✓ Passed  | 6 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 5 s
wallet_multiwallet.py                     | ✓ Passed  | 50 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 15 s
wallet_reorgsrestore.py                   | ✓ Passed  | 4 s
wallet_resendwallettransactions.py        | ✓ Passed  | 4 s
wallet_send.py                            | ✓ Passed  | 8 s
wallet_startup.py                         | ✓ Passed  | 3 s
wallet_timelock.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py                       | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 2 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 3 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s
chronik_serve.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  | 2044 s (accumulated) 
Runtime: 409 s

[160/481] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.006s

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

OK
[217/481] Running seeder test suite
PASSED: seeder test suite
[222/481] Running pow test suite
PASSED: pow test suite
[225/481] Running avalanche test suite
PASSED: avalanche test suite
[228/481] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[230/481] Running secp256k1 test suite
PASSED: secp256k1 test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Tail of the build log:

wallet_address_types.py                   | ✓ Passed  | 12 s
wallet_address_types.py --descriptors     | ✓ Passed  | 7 s
wallet_avoidreuse.py                      | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
wallet_backup.py                          | ✓ Passed  | 17 s
wallet_balance.py                         | ✓ Passed  | 4 s
wallet_balance.py --descriptors           | ✓ Passed  | 7 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  | 7 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  | 13 s
wallet_hd.py                              | ✓ Passed  | 7 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  | 6 s
wallet_importmulti.py                     | ✓ Passed  | 3 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  | 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                     | ✓ Passed  | 38 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 9 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 13 s
wallet_send.py                            | ✓ Passed  | 8 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_serve.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  | 1198 s (accumulated) 
Runtime: 248 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
PiRK requested changes to this revision.Feb 3 2023, 07:21
PiRK added a subscriber: PiRK.

the set vs vector comparison in minerfund_tests is failing

This revision now requires changes to proceed.Feb 3 2023, 07:21

Fix missing vector -> set in the unit test

This revision is now accepted and ready to land.Feb 3 2023, 20:53
Fabien requested changes to this revision.Feb 5 2023, 10:01
Fabien added a subscriber: Fabien.

If ordering matters then it should have a comparator and appropriated rationale/tests

This revision now requires changes to proceed.Feb 5 2023, 10:01
sdulfari requested review of this revision.Feb 6 2023, 19:14

Ordering does not matter at this time. If it did, then the code is broken already. It appears to be designed this way so that the API is more clear if multiple addresses are introduced later.

Fabien requested changes to this revision.Feb 6 2023, 19:35

So you want an unordered_set, and an appropriated hash function to check for equality

This revision now requires changes to proceed.Feb 6 2023, 19:35
Fabien requested changes to this revision.Feb 7 2023, 11:18

Mostly nits.
Mind to add unit tests for the TxDestinationHasher ?

src/node/miner.cpp
201

unrelated but we should pick a random one here rather than the first

src/test/minerfund_tests.cpp
36–39
53–57
src/util/hasher.h
14

why ?

This revision now requires changes to proceed.Feb 7 2023, 11:18
src/test/minerfund_tests.cpp
53–57

expectedMinerFund will be used in additional test cases further up the diff stack so I prefer to leave it like this

src/util/hasher.h
14

leftover from previous iteration of the code

Cleanup and quick unit test for TxDestinationHasher

Fabien requested changes to this revision.Feb 7 2023, 19:09
Fabien added inline comments.
src/test/hasher_tests.cpp
29 ↗(On Diff #37843)

The test is missing the main part: checking that 2 identical CTxDestination return the same hash (with the same hasher).

This revision now requires changes to proceed.Feb 7 2023, 19:09
Fabien requested changes to this revision.Feb 8 2023, 11:08
Fabien added inline comments.
src/test/hasher_tests.cpp
25–28 ↗(On Diff #37849)

You're not testing what you want here, see suggestion

This revision now requires changes to proceed.Feb 8 2023, 11:08
This revision is now accepted and ready to land.Feb 8 2023, 17:25