Page MenuHomePhabricator

[avalanche] set a minimum amount for a proof
Needs ReviewPublic

Authored by PiRK on Tue, Mar 23, 17:27.

Details

Reviewers
majcosta
deadalnix
Group Reviewers
Restricted Project
Summary

This bumps the dust threshold per UTXO in a proof to 1 coin and fixes tests accordingly.

PROOF_DUST_THRESOLD is moved from proof.cpp to proof.h so it is available for tests.

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.Tue, Mar 23, 17:27
PiRK planned changes to this revision.

There are still quite a few unit tests to be changed. We can no longer just take a random uint32 to create a valid proof, it must be in the correct range.

Tail of the build log:

[383/446] bitcoin: testing bip32_tests
[384/446] Running utility command for check-bitcoin-bip32_tests
[385/446] bitcoin: testing settings_tests
[386/446] Running utility command for check-bitcoin-settings_tests
[387/446] bitcoin: testing streams_tests
[388/446] bitcoin: testing txvalidationcache_tests
[389/446] bitcoin: testing timedata_tests
[390/446] Running utility command for check-bitcoin-txvalidationcache_tests
[391/446] Running utility command for check-bitcoin-streams_tests
[392/446] Running utility command for check-bitcoin-timedata_tests
[393/446] bitcoin: testing scriptpubkeyman_tests
[394/446] Running utility command for check-bitcoin-scriptpubkeyman_tests
[395/446] bitcoin: testing uint256_tests
[396/446] bitcoin: testing walletdb_tests
[397/446] Running utility command for check-bitcoin-walletdb_tests
[398/446] Running utility command for check-bitcoin-uint256_tests
[399/446] bitcoin: testing serialize_tests
[400/446] bitcoin: testing script_standard_tests
[401/446] Running utility command for check-bitcoin-serialize_tests
[402/446] bitcoin: testing blockcheck_tests
[403/446] Running utility command for check-bitcoin-script_standard_tests
[404/446] Running utility command for check-bitcoin-blockcheck_tests
[405/446] bitcoin: testing radix_tests
[406/446] Running utility command for check-bitcoin-radix_tests
[407/446] bitcoin: testing schnorr_tests
[408/446] Running utility command for check-pow-aserti32d_tests
[409/446] Running utility command for check-bitcoin-schnorr_tests
[410/446] bitcoin: testing getarg_tests
[411/446] Running pow test suite
PASSED: pow test suite
[412/446] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[413/446] bitcoin: testing wallet_tests
[414/446] Running utility command for check-bitcoin-getarg_tests
[415/446] Running utility command for check-bitcoin-wallet_tests
[416/446] bitcoin: testing versionbits_tests
[417/446] Running utility command for check-bitcoin-versionbits_tests
[418/446] bitcoin: testing script_tests
[419/446] bitcoin: testing monolith_opcodes_tests
[420/446] Running utility command for check-bitcoin-script_tests
[421/446] bitcoin: testing validation_block_tests
[422/446] Running utility command for check-bitcoin-monolith_opcodes_tests
[423/446] Running utility command for check-bitcoin-validation_block_tests
[424/446] bitcoin: testing transaction_tests
[425/446] Running utility command for check-bitcoin-transaction_tests
[426/446] bitcoin: testing skiplist_tests
[427/446] Running utility command for check-bitcoin-skiplist_tests
[428/446] bitcoin: testing coinselector_tests
[429/446] Running utility command for check-bitcoin-coinselector_tests
[430/446] bitcoin: testing op_reversebytes_tests
[431/446] Running utility command for check-bitcoin-op_reversebytes_tests
[432/446] bitcoin: testing coins_tests
[433/446] Running utility command for check-bitcoin-coins_tests
[434/446] Running bitcoin test suite
PASSED: bitcoin test suite
[435/446] secp256k1: testing secp256k1-tests
[436/446] 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

Tail of the build log:

wallet_create_tx.py                              | ○ Skipped | 0 s
wallet_createwallet.py                           | ○ Skipped | 0 s
wallet_createwallet.py --usecli                  | ○ Skipped | 0 s
wallet_descriptor.py                             | ○ Skipped | 0 s
wallet_dump.py                                   | ○ Skipped | 0 s
wallet_encryption.py                             | ○ Skipped | 0 s
wallet_hd.py                                     | ○ Skipped | 0 s
wallet_import_rescan.py                          | ○ Skipped | 0 s
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_keypool.py                                | ○ Skipped | 0 s
wallet_keypool_topup.py                          | ○ Skipped | 0 s
wallet_labels.py                                 | ○ 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_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
wallet_zapwallettxes.py                          | ○ Skipped | 0 s

ALL                                              | ✓ Passed  | 349 s (accumulated) 
Runtime: 74 s

----------------------------------------------------------------------
Ran 4 tests in 0.025s

OK

[148/407] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.004s

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

OK
[157/407] Running pow test suite
PASSED: pow test suite
[168/407] Running seeder test suite
PASSED: seeder test suite
[175/407] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[396/407] 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:

[380/439] bitcoin: testing sighashtype_tests
[381/439] Running utility command for check-bitcoin-merkleblock_tests
[382/439] Running utility command for check-bitcoin-script_commitment_tests
[383/439] bitcoin: testing bip32_tests
[384/439] Running utility command for check-bitcoin-sighashtype_tests
[385/439] bitcoin: testing wallet_crypto_tests
[386/439] bitcoin: testing torcontrol_tests
[387/439] Running utility command for check-bitcoin-bip32_tests
[388/439] Running utility command for check-bitcoin-wallet_crypto_tests
[389/439] Running utility command for check-bitcoin-torcontrol_tests
[390/439] bitcoin: testing settings_tests
[391/439] bitcoin: testing scriptpubkeyman_tests
[392/439] Running utility command for check-bitcoin-settings_tests
[393/439] bitcoin: testing timedata_tests
[394/439] Running utility command for check-bitcoin-scriptpubkeyman_tests
[395/439] bitcoin: testing streams_tests
[396/439] Running utility command for check-bitcoin-timedata_tests
[397/439] bitcoin: testing txvalidationcache_tests
[398/439] Running utility command for check-bitcoin-txvalidationcache_tests
[399/439] Running utility command for check-bitcoin-streams_tests
[400/439] bitcoin: testing uint256_tests
[401/439] Running utility command for check-bitcoin-uint256_tests
[402/439] bitcoin: testing walletdb_tests
[403/439] bitcoin: testing script_standard_tests
[404/439] Running utility command for check-bitcoin-walletdb_tests
[405/439] Running utility command for check-bitcoin-script_standard_tests
[406/439] bitcoin: testing serialize_tests
[407/439] Running utility command for check-bitcoin-serialize_tests
[408/439] bitcoin: testing schnorr_tests
[409/439] bitcoin: testing blockcheck_tests
[410/439] bitcoin: testing transaction_tests
[411/439] Running utility command for check-bitcoin-schnorr_tests
[412/439] Running utility command for check-bitcoin-blockcheck_tests
[413/439] bitcoin: testing radix_tests
[414/439] bitcoin: testing versionbits_tests
[415/439] Running utility command for check-bitcoin-transaction_tests
[416/439] Running utility command for check-bitcoin-radix_tests
[417/439] Running utility command for check-bitcoin-versionbits_tests
[418/439] bitcoin: testing getarg_tests
[419/439] Running utility command for check-bitcoin-getarg_tests
[420/439] bitcoin: testing validation_block_tests
[421/439] Running utility command for check-bitcoin-validation_block_tests
[422/439] bitcoin: testing wallet_tests
[423/439] Running utility command for check-bitcoin-wallet_tests
[424/439] bitcoin: testing skiplist_tests
[425/439] bitcoin: testing script_tests
[426/439] Running utility command for check-bitcoin-skiplist_tests
[427/439] Running utility command for check-bitcoin-script_tests
[428/439] bitcoin: testing monolith_opcodes_tests
[429/439] Running utility command for check-bitcoin-monolith_opcodes_tests
[430/439] bitcoin: testing coinselector_tests
[431/439] Running utility command for check-bitcoin-coinselector_tests
[432/439] bitcoin: testing op_reversebytes_tests
[433/439] Running utility command for check-bitcoin-op_reversebytes_tests
[434/439] bitcoin: testing coins_tests
[435/439] Running utility command for check-bitcoin-coins_tests
[436/439] 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_createwallet.py                           | ✓ Passed  | 2 s
wallet_createwallet.py --usecli                  | ✓ Passed  | 3 s
wallet_descriptor.py                             | ✓ Passed  | 9 s
wallet_disable.py                                | ✓ Passed  | 1 s
wallet_dump.py                                   | ✓ Passed  | 5 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_hd.py                                     | ✓ Passed  | 7 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  | 5 s
wallet_importprunedfunds.py                      | ✓ Passed  | 1 s
wallet_keypool.py                                | ✓ Passed  | 3 s
wallet_keypool_topup.py                          | ✓ Passed  | 3 s
wallet_labels.py                                 | ✓ Passed  | 2 s
wallet_listreceivedby.py                         | ✓ Passed  | 29 s
wallet_listsinceblock.py                         | ✓ Passed  | 6 s
wallet_listtransactions.py                       | ✓ Passed  | 15 s
wallet_multiwallet.py                            | ✓ Passed  | 13 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 14 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 5 s
wallet_txn_clone.py                              | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock                  | ✓ Passed  | 3 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  | 1 s
wallet_zapwallettxes.py                          | ✓ Passed  | 4 s

ALL                                              | ✓ Passed  | 1156 s (accumulated) 
Runtime: 234 s

----------------------------------------------------------------------
Ran 4 tests in 0.002s

OK

[150/447] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.006s

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

OK
[183/447] Running seeder test suite
PASSED: seeder test suite
[193/447] Running pow test suite
PASSED: pow test suite
[431/447] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[436/447] 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_preciousblock.py                             | ✓ Passed  | 1 s
rpc_psbt.py                                      | ✓ Passed  | 21 s
rpc_rawtransaction.py                            | ✓ Passed  | 32 s
rpc_scantxoutset.py                              | ✓ Passed  | 3 s
rpc_setban.py                                    | ✓ Passed  | 2 s
rpc_signmessage.py                               | ✓ Passed  | 1 s
rpc_signrawtransaction.py                        | ✓ Passed  | 1 s
rpc_txoutproof.py                                | ✓ Passed  | 2 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  | 10 s
wallet_address_types.py                          | ✓ Passed  | 12 s
wallet_avoidreuse.py                             | ✓ Passed  | 3 s
wallet_backup.py                                 | ✓ Passed  | 25 s
wallet_balance.py                                | ✓ Passed  | 18 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  | 1 s
wallet_dump.py                                   | ✓ Passed  | 4 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_hd.py                                     | ✓ Passed  | 6 s
wallet_import_rescan.py                          | ✓ Passed  | 5 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  | 1 s
wallet_keypool.py                                | ✓ Passed  | 3 s
wallet_keypool_topup.py                          | ✓ Passed  | 3 s
wallet_labels.py                                 | ✓ Passed  | 1 s
wallet_listreceivedby.py                         | ✓ Passed  | 18 s
wallet_listsinceblock.py                         | ✓ Passed  | 4 s
wallet_listtransactions.py                       | ✓ Passed  | 7 s
wallet_multiwallet.py                            | ✓ Passed  | 11 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 12 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 22 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
wallet_zapwallettxes.py                          | ✓ Passed  | 3 s

ALL                                              | ✓ Passed  | 830 s (accumulated) 
Runtime: 169 s

----------------------------------------------------------------------
Ran 4 tests in 0.001s

OK

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

set only a single limit (1 coin per utxo) because sybill resistance is already achieved by weighting peer selection probability based on its staked amount

Remove the INVALID_TOTAL_AMOUNT concept, it does not provide much value (even with this MAX_MONEY limit in place, malicious nodes can still create invalid proofs that can only be discarded when checking individual UTXOs)

Rebase on D9351

remove accidentaly duplicated test and restore deleted blank line

majcosta requested changes to this revision.Fri, Mar 26, 14:17
majcosta added a subscriber: majcosta.
majcosta added inline comments.
src/avalanche/test/peermanager_tests.cpp
311 ↗(On Diff #28027)

if MIN_SCORE_DUST is 100, this should be 1000000 * MIN_SCORE_DUST or changes test behavior

350 ↗(On Diff #28027)

should be MIN_SCORE_DUST/100, no?

This revision now requires changes to proceed.Fri, Mar 26, 14:17
src/avalanche/test/peermanager_tests.cpp
311 ↗(On Diff #28027)

I haven't studied how the score affects the probability a node gets selected, but I assumed that it is proportional to the value staked. So in this case, if I multiply the previous test values by the same constant, it should give the same result.

350 ↗(On Diff #28027)

It has to be a valid proof as low as possible. MIN_SCORE is meant to be the corresponding score to that lowest possible stake.

src/avalanche/test/peermanager_tests.cpp
350 ↗(On Diff #28027)

The buildRandomProof function takes a score, not an amount.

src/avalanche/test/peermanager_tests.cpp
349 ↗(On Diff #28027)

If I understand the test correctly, this comment is incorrect. It should be "1 in 100 million"
Can someone confirm that they understand the same thing?

PiRK requested review of this revision.Fri, Mar 26, 16:42
deadalnix requested changes to this revision.Thu, Apr 1, 12:25
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/proof.h
20 ↗(On Diff #28027)

Why isn't this in the namespace?

src/avalanche/test/util.h
14 ↗(On Diff #28027)

dito

14 ↗(On Diff #28027)

Also MIN_SCORE_DUST doesn't mean anything. Does the dust has a score now?

This revision now requires changes to proceed.Thu, Apr 1, 12:25

mmove constexprs into the avalanche namespace
rename MIN_SCORE_DUST -> SCORE_DUST_THRESHOLD

deadalnix requested changes to this revision.Sat, Apr 3, 20:37
deadalnix added inline comments.
src/avalanche/test/peermanager_tests.cpp
165 ↗(On Diff #28064)

A lot of these test are not testing for the dust threshold, right? So SCORE_DUST_THRESHOLD isn't actually what you want? So what is it that you want? Maybe the minimal score that would generate a valid proof?

src/avalanche/test/proof_tests.cpp
29–37 ↗(On Diff #28064)

You could make this way more generic without doing much of anything.

30–37 ↗(On Diff #28064)
443 ↗(On Diff #28064)

Now you are testing the dust threshold, so this is what you want.

This revision now requires changes to proceed.Sat, Apr 3, 20:37
src/avalanche/test/peermanager_tests.cpp
165 ↗(On Diff #28064)

Maybe the minimal score that would generate a valid proof?

That's exactly what MIN_SCORE_DUST / SCORE_DUST_THRESHOLD was meant to mean.

refactor conditional check of ProofValidationResult as suggested in the review

I had to BOOST_CHECK(state.GetResult() == expected_state); instead of BOOST_CHECK_EQUAL(state.GetResult(), expected_state);.
The latter causes a very long stack of compilation errors because the "Enable BOOST_CHECK_EQUAL for enum class types" trick in setyp_common.h does not appear to work in this case.

deadalnix requested changes to this revision.Wed, Apr 7, 15:40
deadalnix added inline comments.
src/avalanche/test/peermanager_tests.cpp
165 ↗(On Diff #28073)

Does it matter that this is the proof dust threshold?

I don't think it does.

This revision now requires changes to proceed.Wed, Apr 7, 15:40

SCORE_DUST_THRESHOLD -> LOWEST_VALID_SCORE

deadalnix requested changes to this revision.Thu, Apr 8, 13:18
deadalnix added inline comments.
src/avalanche/test/proof_tests.cpp
30 ↗(On Diff #28083)

What you want here is NOT the lowest valid score.

Please. These two things have the same value, but they are NOT the same thing. The whole reason you give it a name to begin with is to identify what that is.

This revision now requires changes to proceed.Thu, Apr 8, 13:18

new attempt at getting this unstuck: restore the proof_random test as close as possible to its initial state, don't test for the ProofValidationResult::DUST_THRESOLD, just make sure the score given to buildRandomProof is always valid.

deadalnix requested changes to this revision.Wed, Apr 14, 15:31

On a side, note, if we commit to the proof score being the amount in satoshi, how do we handle overflows?

src/avalanche/test/proof_tests.cpp
25 ↗(On Diff #28183)

This is going to skew the distribution quite considerably.

I don't think there a major issue with tripping the dust threshold error path with randomly generated proofs. is there a reason we want to avoid doing that?

343 ↗(On Diff #28183)

Is there a reason to make this amount dependent on the dust threshold?

437 ↗(On Diff #28183)

Why not 1 * SATOSHI ?

src/avalanche/test/util.h
16 ↗(On Diff #28183)
// Minimum score for the proof to be valid

Then shouldn't this be something alike to MIN_VALID_PROOF_SCORE ?

Clearly, you thought the name was confusing because you added a comment. So make the name explicit.

test/functional/abc_rpc_avalancheproof.py
114 ↗(On Diff #28183)

likestamp

This revision now requires changes to proceed.Wed, Apr 14, 15:31
src/avalanche/test/proof_tests.cpp
25 ↗(On Diff #28183)

My idea was not to change the actual meaning of this test. With the previous change, it tested more stuff than previously. Instead of just testing that valid proofs verify successfuly, it tested ProofValidationStates of invalid proofs.

But I see what you mean, now it's not as random anymore.

I'm still not sure what name[[ https://reviews.bitcoinabc.org/D9345?id=28083#inline-46121 | you think would be better for the threshold ]], if we go back to testing dust thresholds.

343 ↗(On Diff #28183)

It's a guarantee that it will remain valid in the future if the threshold is changes.

rename LOWEST_VALID_SCORE -> MIN_VALID_PROOF_SCORE

In the proof_random test, don't avoid tripping the dust threshold error path. Add a function to test whether a given proof will trip the path, independently of how the proof is built. This removes the assumption that buildAvalancheProof will always generate single stake proofs with their value only depending on the score.