Page MenuHomePhabricator
Differential D10524

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

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 created this revision.Nov 24 2021, 16:48
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 24 2021, 16:48
Fabien requested review of this revision.Nov 24 2021, 16:48
Fabien added a child revision: D10525: [avalanche] Make it possible to replace a proof.Nov 24 2021, 16:58
Harbormaster completed remote builds in B17337: Diff 30989.Nov 24 2021, 17:00
Fabien planned changes to this revision.Nov 26 2021, 15:01
deadalnix added a subscriber: deadalnix.Nov 26 2021, 15:01
deadalnix added inline comments.
src/avalanche/peermanager.h
296 ↗(On Diff #30989)

bool allowOverride = true ?

Fabien updated this revision to Diff 31148.Nov 29 2021, 16:51

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

Harbormaster completed remote builds in B17445: Diff 31148.Nov 29 2021, 17:35
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
Fabien mentioned this in D10522: [avalanche] Add wrappers to add and remove a proof from a proof pool.Nov 30 2021, 13:03
Fabien updated this revision to Diff 31171.Nov 30 2021, 15:03

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

Harbormaster completed remote builds in B17463: Diff 31171.Nov 30 2021, 15:13
teamcity added a comment.Nov 30 2021, 15:13
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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 added a comment.Nov 30 2021, 20:13

Restart the build

Fabien updated this revision to Diff 31195.Dec 1 2021, 09:41

Rebase

Harbormaster completed remote builds in B17478: Diff 31195.Dec 1 2021, 10:01
Fabien planned changes to this revision.Dec 1 2021, 11:02
Fabien updated this revision to Diff 31201.Dec 1 2021, 11:11

Deduplicate the code a bit.

Harbormaster completed remote builds in B17484: Diff 31201.Dec 1 2021, 11:25
Fabien updated this revision to Diff 31210.Dec 1 2021, 15:05

Rebase

Fabien added a child revision: D10605: [avalanche] Move the proof registration time from the peer to the proof pool.Dec 1 2021, 15:09
Harbormaster completed remote builds in B17492: Diff 31210.Dec 1 2021, 15:21
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
Fabien added inline comments.Dec 7 2021, 16:13
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.

Fabien updated this revision to Diff 31306.Dec 7 2021, 16:51

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.

Harbormaster completed remote builds in B17565: Diff 31306.Dec 7 2021, 17:08
Fabien planned changes to this revision.Dec 8 2021, 17:23

Will split

Fabien updated this revision to Diff 31342.Dec 9 2021, 16:56

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 edited the summary of this revision. (Show Details)Dec 9 2021, 16:56
Fabien added parent revisions: D10658: [avalanche] Create a conflicting proof pool, D10651: [avalanche] Add a facility to replace a proof in the pool.
Fabien removed a parent revision: D10523: [avalanche] Use the ProofPool multi index to store the orphan proofs.
Fabien planned changes to this revision.Dec 9 2021, 17:01

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

Harbormaster completed remote builds in B17596: Diff 31342.Dec 9 2021, 17:08
Fabien edited the summary of this revision. (Show Details)Dec 10 2021, 09:40
Fabien updated this revision to Diff 31347.Dec 10 2021, 09:41

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

Fabien added a child revision: D10661: [avalanche] Select the favorite orphan in case of a conflict during a rescan.Dec 10 2021, 09:44
Harbormaster completed remote builds in B17601: Diff 31347.Dec 10 2021, 09:52
deadalnix accepted this revision.Dec 10 2021, 16:37
This revision is now accepted and ready to land.Dec 10 2021, 16:37
Closed by commit rABC0d774d1e49b3: [avalanche] Select the favorite orphan in case of a conflict (authored by Fabien). · Explain WhyDec 13 2021, 09:15
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC0d774d1e49b3: [avalanche] Select the favorite orphan in case of a conflict.
Fabien removed a child revision: D10525: [avalanche] Make it possible to replace a proof.Dec 14 2021, 15:55

Revision Contents
Changeset List

PathSize
src/
avalanche/
8 lines
20 lines
44 lines
test/
5 lines
84 lines

Diff 31201

View Options

src/avalanche/peermanager.cpp

Loading...
View Options

src/avalanche/proofpool.h

Loading...
View Options

src/avalanche/proofpool.cpp

Loading...
View Options

src/avalanche/test/peermanager_tests.cpp

Loading...
View Options

src/avalanche/test/proofpool_tests.cpp

Loading...