Page MenuHomePhabricator

[avalanche] Select the favorite orphan in case of a conflict
ClosedPublic

Authored by Fabien on Nov 24 2021, 16:48.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC0d774d1e49b3: [avalanche] Select the favorite orphan in case of a conflict
Summary

If there is a conflict in the orphan pool, there is no reason to not store the best candidate.
This diff only applies the selection for the proofs added via the registration method. The case of valid proofs being orphaned due to a UTXO being spent will be covered in a follow-up.

Ref T1854.

Depends on D10651 and D10658.

Test Plan
ninja all check-avalanche

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_peermanager_replace_conflicting_orphans
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17484
Build 34794: Build Diffbuild-diff · build-debug · build-clang · lint-circular-dependencies · build-without-wallet · build-clang-tidy
Build 34793: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Nov 24 2021, 16:48
Fabien planned changes to this revision.Nov 26 2021, 15:01
deadalnix added inline comments.
src/avalanche/peermanager.h
296 ↗(On Diff #30989)

bool allowOverride = true ?

Rebase, split into 2 methods, addProof and addProofNoOverride to avoid the boolean

deadalnix requested changes to this revision.Nov 29 2021, 22:20

The behavior of addProof changed, yet no test changed. This is suspicious.

src/avalanche/peermanager.cpp
258 ↗(On Diff #31148)

It's hard to argue that this is an improvement.

src/avalanche/proofpool.h
73 ↗(On Diff #31148)

It's really unclear what override means here. And why would the other doesn't mention it. I can't figure out what this is doing by looking at the API, which is a bad sign.

This revision now requires changes to proceed.Nov 29 2021, 22:20

Rename the methods: addProof => addProofIfPreferred and addProofNoOverride => addProofIfNoConflict.
This is verbose but hopefully explicit enough. Also added doxygen comments for these.

Tail of the build log:

rpc_signmessage.py                      | ✓ Passed  | 1 s
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  | 12 s
wallet_address_types.py                 | ✓ Passed  | 15 s
wallet_avoidreuse.py                    | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors      | ✓ Passed  | 4 s
wallet_backup.py                        | ✓ Passed  | 24 s
wallet_balance.py                       | ✓ Passed  | 10 s
wallet_basic.py                         | ✓ Passed  | 21 s
wallet_coinbase_category.py             | ✓ Passed  | 1 s
wallet_create_tx.py                     | ✓ Passed  | 6 s
wallet_createwallet.py                  | ✓ Passed  | 3 s
wallet_createwallet.py --usecli         | ✓ Passed  | 3 s
wallet_descriptor.py                    | ✓ Passed  | 7 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  | 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  | 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  | 3 s
wallet_keypool_topup.py --descriptors   | ✓ Passed  | 3 s
wallet_labels.py                        | ✓ Passed  | 2 s
wallet_labels.py --descriptors          | ✓ Passed  | 3 s
wallet_listreceivedby.py                | ✓ Passed  | 11 s
wallet_listsinceblock.py                | ✓ Passed  | 4 s
wallet_listtransactions.py              | ✓ Passed  | 28 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 12 s
wallet_reorgsrestore.py                 | ✓ Passed  | 4 s
wallet_resendwallettransactions.py      | ✓ Passed  | 2 s
wallet_send.py                          | ✓ Passed  | 8 s
wallet_startup.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py                     | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 4 s
wallet_txn_doublespend.py               | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 4 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 1 s

ALL                                     | ✓ Passed  | 975 s (accumulated) 
Runtime: 195 s

----------------------------------------------------------------------
Ran 10 tests in 0.094s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
Fabien planned changes to this revision.Dec 1 2021, 11:02

Deduplicate the code a bit.

deadalnix requested changes to this revision.Dec 7 2021, 14:21
deadalnix added inline comments.
src/avalanche/proofpool.cpp
16 ↗(On Diff #31210)

You should put this kind of dummy functions in the header.

38 ↗(On Diff #31210)

You don't know if the set was empty to begin with, so this is not a valid check anymore.

You probably need ot add check with set that are non empty to make sure the behavior is what you expect.

59 ↗(On Diff #31210)

This should be moved to the header too.

69 ↗(On Diff #31210)

Once again, this check is not valid if the set wasn't empty to begin with.

src/avalanche/test/peermanager_tests.cpp
973 ↗(On Diff #31210)

Why is there a behavioral change here?

This revision now requires changes to proceed.Dec 7 2021, 14:21
src/avalanche/proofpool.cpp
38 ↗(On Diff #31210)

Good catch

src/avalanche/test/peermanager_tests.cpp
973 ↗(On Diff #31210)

This is testing conflicting orphans, and this behavior is changed by this diff so the test needs to be updated.

Move the dummy functions to the header, clear the set to make sure it's empty before we insert anything in it and add a test case to cover this.

Fabien planned changes to this revision.Dec 8 2021, 17:23

Will split

Split apart the pool facitilies so only the feature change remains.
Depends on D10658 that also splitted appart some of the orphan pool mechanism into a new conflicting pool.

Fabien planned changes to this revision.Dec 9 2021, 17:01

Need to add a test case for the override coming from a rescan

Manage the rescan in another diff so it is easier to review. Summary updated accordingly.

This revision is now accepted and ready to land.Dec 10 2021, 16:37