Page MenuHomePhabricator

[avalanche] Introduce ProofRef as an alias for std::shared_ptr<Proof>
ClosedPublic

Authored by Fabien on Oct 14 2021, 12:22.

Details

Reviewers
PiRK
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC4df517ddfb86: [avalanche] Introduce ProofRef as an alias for std::shared_ptr<Proof>
Summary

This is just shorter and more readable. There is no change in behavior.

Depends on D10355.

Ref T1854.

Test Plan
ninja all check-all

Should return a single occurrence (the definition):

git grep "std::shared_ptr<Proof>"

Should return no occurrence:

git grep "std::shared_ptr<avalanche::Proof>"

Diff Detail

Event Timeline

Fabien requested review of this revision.Oct 14 2021, 12:22

Tail of the build log:

rpc_signrawtransaction.py               | ✓ Passed  | 2 s
rpc_txoutproof.py                       | ✓ Passed  | 1 s
rpc_uptime.py                           | ✓ Passed  | 1 s
rpc_users.py                            | ✓ Passed  | 5 s
rpc_whitelist.py                        | ✓ Passed  | 1 s
tool_wallet.py                          | ✓ Passed  | 4 s
wallet_abandonconflict.py               | ✓ Passed  | 17 s
wallet_address_types.py                 | ✓ Passed  | 13 s
wallet_avoidreuse.py                    | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors      | ✓ Passed  | 4 s
wallet_backup.py                        | ✓ Passed  | 25 s
wallet_balance.py                       | ✓ Passed  | 22 s
wallet_basic.py                         | ✓ Passed  | 18 s
wallet_coinbase_category.py             | ✓ Passed  | 1 s
wallet_create_tx.py                     | ✓ Passed  | 5 s
wallet_createwallet.py                  | ✓ Passed  | 2 s
wallet_createwallet.py --usecli         | ✓ Passed  | 3 s
wallet_descriptor.py                    | ✓ Passed  | 5 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_hd.py                            | ✓ Passed  | 6 s
wallet_hd.py --descriptors              | ✓ Passed  | 5 s
wallet_import_rescan.py                 | ✓ Passed  | 4 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importdescriptors.py             | ✓ Passed  | 4 s
wallet_importmulti.py                   | ✓ Passed  | 3 s
wallet_importprunedfunds.py             | ✓ Passed  | 2 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 2 s
wallet_keypool_topup.py --descriptors   | ✓ Passed  | 3 s
wallet_labels.py                        | ✓ Passed  | 1 s
wallet_labels.py --descriptors          | ✓ Passed  | 2 s
wallet_listreceivedby.py                | ✓ Passed  | 10 s
wallet_listsinceblock.py                | ✓ Passed  | 5 s
wallet_listtransactions.py              | ✓ Passed  | 20 s
wallet_multiwallet.py                   | ✓ Passed  | 39 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 11 s
wallet_reorgsrestore.py                 | ✓ Passed  | 3 s
wallet_resendwallettransactions.py      | ✓ Passed  | 4 s
wallet_send.py                          | ✓ Passed  | 7 s
wallet_startup.py                       | ✓ Passed  | 2 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

ALL                                     | ✓ Passed  | 1030 s (accumulated) 
Runtime: 211 s

----------------------------------------------------------------------
Ran 9 tests in 0.093s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

Unrelated rare leveldb failure, restarting the tests

This revision is now accepted and ready to land.Oct 14 2021, 12:50
deadalnix requested changes to this revision.Oct 14 2021, 15:17
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/proof.h
156 ↗(On Diff #30536)

CTransactionRef references a const CTransaction, is there a reason not to do the same here?

src/avalanche/test/peermanager_tests.cpp
177 ↗(On Diff #30536)

If you make the type more opaque, then wrap this as well. This code makes no sense in the current state of affair, because it pretends to abstract away the use of the shared_ptr but doesn't in this case.

This revision now requires changes to proceed.Oct 14 2021, 15:17
src/avalanche/proof.h
156 ↗(On Diff #30536)

Yes, there is one spot that needs a small update for that to work (in the processor factory we pass the peer data proof as a reference to be initialized from the -avaproof hex string). This will better be done in a follow-up

src/avalanche/proofcomparator.h
18 ↗(On Diff #30536)

This one deserves to be renamed as well

src/avalanche/test/peermanager_tests.cpp
177 ↗(On Diff #30536)

I should just remove this one and update buildRandomProof to return a shared pointer, since most of the API uses that. Will do in another diff.

Rebase on top of D10349 (removed the getRandomProofPtr() wrapper), rename ProofSharedPointerComparator to ProofRefComparator

I mistakenly removed an extra line during the rebase, and this was a useful line.

Tail of the build log:

[385/455] Running utility command for check-bitcoin-txvalidationcache_tests
[386/455] Running utility command for check-bitcoin-finalization_tests
[387/455] bitcoin: testing merkleblock_tests
[388/455] bitcoin: testing op_reversebytes_tests
[389/455] Running utility command for check-bitcoin-merkleblock_tests
[390/455] bitcoin: testing scheduler_tests
[391/455] Running utility command for check-bitcoin-op_reversebytes_tests
[392/455] bitcoin: testing bip32_tests
[393/455] Running utility command for check-bitcoin-scheduler_tests
[394/455] Running utility command for check-bitcoin-bip32_tests
[395/455] bitcoin: testing sync_tests
[396/455] Running utility command for check-bitcoin-sync_tests
[397/455] bitcoin: testing torcontrol_tests
[398/455] bitcoin: testing transaction_tests
[399/455] Running utility command for check-bitcoin-torcontrol_tests
[400/455] bitcoin: testing settings_tests
[401/455] Running utility command for check-bitcoin-transaction_tests
[402/455] Running utility command for check-bitcoin-settings_tests
[403/455] bitcoin: testing streams_tests
[404/455] bitcoin: testing timedata_tests
[405/455] Running utility command for check-bitcoin-streams_tests
[406/455] Running utility command for check-bitcoin-timedata_tests
[407/455] bitcoin: testing compilerbug_tests
[408/455] bitcoin: testing validation_flush_tests
[409/455] bitcoin: testing blockcheck_tests
[410/455] Running utility command for check-bitcoin-compilerbug_tests
[411/455] Running utility command for check-bitcoin-validation_flush_tests
[412/455] Running utility command for check-bitcoin-blockcheck_tests
[413/455] bitcoin: testing checkpoints_tests
[414/455] Running utility command for check-bitcoin-checkpoints_tests
[415/455] bitcoin: testing intmath_tests
[416/455] bitcoin: testing serialize_tests
[417/455] bitcoin: testing schnorr_tests
[418/455] Running utility command for check-bitcoin-intmath_tests
[419/455] bitcoin: testing validationinterface_tests
[420/455] Running utility command for check-bitcoin-serialize_tests
[421/455] Running utility command for check-bitcoin-schnorr_tests
[422/455] bitcoin: testing wallet_tests
[423/455] Running utility command for check-bitcoin-validationinterface_tests
[424/455] bitcoin: testing blockstatus_tests
[425/455] bitcoin: testing cashaddr_tests
[426/455] Running utility command for check-bitcoin-wallet_tests
[427/455] Running utility command for check-bitcoin-blockstatus_tests
[428/455] Running utility command for check-bitcoin-cashaddr_tests
[429/455] bitcoin: testing versionbits_tests
[430/455] Running utility command for check-bitcoin-versionbits_tests
[431/455] bitcoin: testing crypto_tests
[432/455] Running utility command for check-bitcoin-crypto_tests
[433/455] bitcoin: testing script_tests
[434/455] Running utility command for check-bitcoin-script_tests
[435/455] bitcoin: testing monolith_opcodes_tests
[436/455] bitcoin: testing coinselector_tests
[437/455] Running utility command for check-bitcoin-monolith_opcodes_tests
[438/455] Running utility command for check-bitcoin-coinselector_tests
[439/455] bitcoin: testing coins_tests
[440/455] Running utility command for check-bitcoin-coins_tests
[441/455] Running bitcoin test suite
PASSED: bitcoin 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:

[378/448] Running utility command for check-bitcoin-bip32_tests
[379/448] Running utility command for check-bitcoin-txvalidationcache_tests
[380/448] bitcoin: testing sync_tests
[381/448] bitcoin: testing torcontrol_tests
[382/448] Running utility command for check-bitcoin-sync_tests
[383/448] bitcoin: testing op_reversebytes_tests
[384/448] Running utility command for check-bitcoin-torcontrol_tests
[385/448] Running utility command for check-bitcoin-op_reversebytes_tests
[386/448] bitcoin: testing settings_tests
[387/448] bitcoin: testing streams_tests
[388/448] Running utility command for check-bitcoin-settings_tests
[389/448] Running utility command for check-bitcoin-streams_tests
[390/448] bitcoin: testing timedata_tests
[391/448] Running utility command for check-bitcoin-timedata_tests
[392/448] bitcoin: testing transaction_tests
[393/448] bitcoin: testing serialize_tests
[394/448] bitcoin: testing radix_tests
[395/448] bitcoin: testing blockcheck_tests
[396/448] Running utility command for check-bitcoin-transaction_tests
[397/448] Running utility command for check-bitcoin-serialize_tests
[398/448] Running utility command for check-bitcoin-blockcheck_tests
[399/448] Running utility command for check-bitcoin-radix_tests
[400/448] bitcoin: testing validation_flush_tests
[401/448] Running utility command for check-bitcoin-validation_flush_tests
[402/448] bitcoin: testing compilerbug_tests
[403/448] bitcoin: testing schnorr_tests
[404/448] Running utility command for check-bitcoin-compilerbug_tests
[405/448] Running utility command for check-bitcoin-schnorr_tests
[406/448] bitcoin: testing wallet_tests
[407/448] bitcoin: testing checkpoints_tests
[408/448] bitcoin: testing script_standard_tests
[409/448] Running utility command for check-bitcoin-wallet_tests
[410/448] Running utility command for check-bitcoin-checkpoints_tests
[411/448] Running utility command for check-bitcoin-script_standard_tests
[412/448] bitcoin: testing validationinterface_tests
[413/448] Running utility command for check-bitcoin-validationinterface_tests
[414/448] bitcoin: testing blockstatus_tests
[415/448] bitcoin: testing cashaddr_tests
[416/448] Running utility command for check-bitcoin-blockstatus_tests
[417/448] Running utility command for check-bitcoin-cashaddr_tests
[418/448] bitcoin: testing script_tests
[419/448] bitcoin: testing crypto_tests
[420/448] Running utility command for check-bitcoin-script_tests
[421/448] Running utility command for check-bitcoin-crypto_tests
[422/448] bitcoin: testing versionbits_tests
[423/448] Running utility command for check-bitcoin-versionbits_tests
[424/448] bitcoin: testing intmath_tests
[425/448] bitcoin: testing util_tests
[426/448] Running utility command for check-bitcoin-intmath_tests
[427/448] Running utility command for check-bitcoin-util_tests
[428/448] bitcoin: testing coinselector_tests
[429/448] Running utility command for check-bitcoin-coinselector_tests
[430/448] bitcoin: testing monolith_opcodes_tests
[431/448] Running utility command for check-bitcoin-monolith_opcodes_tests
[432/448] bitcoin: testing coins_tests
[433/448] Running utility command for check-bitcoin-coins_tests
[434/448] Running bitcoin test suite
PASSED: bitcoin 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:

wallet_importmulti.py                   | ○ Skipped | 0 s
wallet_importprunedfunds.py             | ○ 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_listtransactions.py              | ○ Skipped | 0 s
wallet_multiwallet.py                   | ○ 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_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  | 567 s (accumulated) 
Runtime: 114 s

----------------------------------------------------------------------
Ran 9 tests in 0.158s

OK

[151/416] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

OK
[152/416] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[160/416] Running pow test suite
PASSED: pow test suite
[174/416] Running seeder test suite
PASSED: seeder test suite
[177/416] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[178/416] 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
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
[401/416] Running bitcoin test suite
PASSED: bitcoin 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_hd.py                            | ✓ Passed  | 8 s
wallet_hd.py --descriptors              | ✓ Passed  | 7 s
wallet_import_rescan.py                 | ✓ Passed  | 10 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importdescriptors.py             | ✓ Passed  | 8 s
wallet_importmulti.py                   | ✓ Passed  | 6 s
wallet_importprunedfunds.py             | ✓ Passed  | 2 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 4 s
wallet_keypool_topup.py --descriptors   | ✓ Passed  | 4 s
wallet_labels.py                        | ✓ Passed  | 2 s
wallet_labels.py --descriptors          | ✓ Passed  | 2 s
wallet_listreceivedby.py                | ✓ Passed  | 44 s
wallet_listsinceblock.py                | ✓ Passed  | 5 s
wallet_listtransactions.py              | ✓ Passed  | 13 s
wallet_multiwallet.py                   | ✓ Passed  | 30 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 17 s
wallet_reorgsrestore.py                 | ✓ Passed  | 5 s
wallet_resendwallettransactions.py      | ✓ Passed  | 10 s
wallet_send.py                          | ✓ Passed  | 6 s
wallet_startup.py                       | ✓ Passed  | 3 s
wallet_txn_clone.py                     | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 4 s
wallet_txn_doublespend.py               | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 4 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 2 s

ALL                                     | ✓ Passed  | 1592 s (accumulated) 
Runtime: 321 s

----------------------------------------------------------------------
Ran 9 tests in 0.265s

OK

[173/456] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.017s

OK
[175/456] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[185/456] Running seeder test suite
PASSED: seeder test suite
[353/456] Running secp256k1 test suite
PASSED: secp256k1 test suite
[414/456] Running pow test suite
PASSED: pow test suite
[436/456] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[441/456] Running bitcoin test suite
PASSED: bitcoin 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:

rpc_signrawtransaction.py               | ✓ Passed  | 1 s
rpc_txoutproof.py                       | ✓ Passed  | 1 s
rpc_uptime.py                           | ✓ Passed  | 1 s
rpc_users.py                            | ✓ Passed  | 5 s
rpc_whitelist.py                        | ✓ Passed  | 1 s
tool_wallet.py                          | ✓ Passed  | 4 s
wallet_abandonconflict.py               | ✓ Passed  | 16 s
wallet_address_types.py                 | ✓ Passed  | 12 s
wallet_avoidreuse.py                    | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors      | ✓ Passed  | 5 s
wallet_backup.py                        | ✓ Passed  | 25 s
wallet_balance.py                       | ✓ Passed  | 9 s
wallet_basic.py                         | ✓ Passed  | 22 s
wallet_coinbase_category.py             | ✓ Passed  | 1 s
wallet_create_tx.py                     | ✓ Passed  | 5 s
wallet_createwallet.py                  | ✓ 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_hd.py                            | ✓ Passed  | 6 s
wallet_hd.py --descriptors              | ✓ Passed  | 5 s
wallet_import_rescan.py                 | ✓ Passed  | 4 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_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 2 s
wallet_keypool_topup.py --descriptors   | ✓ Passed  | 3 s
wallet_labels.py                        | ✓ Passed  | 2 s
wallet_labels.py --descriptors          | ✓ Passed  | 2 s
wallet_listreceivedby.py                | ✓ Passed  | 29 s
wallet_listsinceblock.py                | ✓ Passed  | 3 s
wallet_listtransactions.py              | ✓ Passed  | 10 s
wallet_multiwallet.py                   | ✓ Passed  | 39 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 11 s
wallet_reorgsrestore.py                 | ✓ Passed  | 3 s
wallet_resendwallettransactions.py      | ✓ Passed  | 2 s
wallet_send.py                          | ✓ Passed  | 8 s
wallet_startup.py                       | ✓ Passed  | 2 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

ALL                                     | ✓ Passed  | 974 s (accumulated) 
Runtime: 200 s

----------------------------------------------------------------------
Ran 9 tests in 0.093s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

Despite the teamcity comments, the builds are green (see the previous comment, I fixed the issue before the CI completed the builds)

deadalnix requested changes to this revision.Oct 14 2021, 20:02
deadalnix added inline comments.
src/avalanche/proof.h
158 ↗(On Diff #30554)

Like for the previous version, why isn't the proof const?

src/avalanche/proofcomparator.h
18 ↗(On Diff #30554)

This is a bit sketchy. Make this a proof compactors and support both const Proof& and shared pointer, it really doesn't cost anything.

This revision now requires changes to proceed.Oct 14 2021, 20:02
src/avalanche/proof.h
158 ↗(On Diff #30554)

I answered that above but I guess it was lost in the flood of CI messages:

there is one spot that needs a small update for that to work (in the processor factory we pass the peer data proof as a reference to be initialized from the -avaproof hex string). This will better be done in a follow-up

src/avalanche/proofcomparator.h
18 ↗(On Diff #30554)

Done in D10355

src/avalanche/proof.h
158 ↗(On Diff #30554)

See D10356

This revision is now accepted and ready to land.Oct 15 2021, 11:44
src/avalanche/proof.h
158 ↗(On Diff #30554)

You really need to fix things first instead of doing the wrong thing and then fixing it in a subsequent patch. This is backward.