Page MenuHomePhabricator
Differential D10522

[avalanche] Add wrappers to add and remove a proof from a proof pool
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC799f65f976aa: [avalanche] Add wrappers to add and remove a proof from a proof pool
Summary

This will make it easier to use the ProofPool structure to store the orphans. It makes the ProofPool a class, but the internal container is kept public to avoid bloating the diff with more changes. Accessors can be added as needed later to make it private. There is no change in behavior.

Ref T1854.

Depends on D10582.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.Nov 24 2021, 16:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 24 2021, 16:24
Fabien requested review of this revision.Nov 24 2021, 16:24
Harbormaster completed remote builds in B17335: Diff 30987.Nov 24 2021, 16:36
Fabien added a child revision: D10523: [avalanche] Use the ProofPool multi index to store the orphan proofs.Nov 24 2021, 16:42
deadalnix requested changes to this revision.Nov 25 2021, 22:17
deadalnix added a subscriber: deadalnix.

Is it something we want at all now that we don't want to store the orphan in the proof pool? Also, it seems like you are slowly recreating fetchOrCreatePeer with extra steps.

src/avalanche/peermanager.cpp
162 ↗(On Diff #30987)

This whole contraption doesn't make a lot of sense does it? state is passed down, but it's useless, because it is captured by the lambda. I assume you wanted WITH_LOCK

264 ↗(On Diff #30987)

You really want to check if this thing is in the pool. If it already is, then addToPool will remove it.

291 ↗(On Diff #30987)

I you pass a reference from a proof that is itself within the proof pool, you might destroy it before this function terminates. It's probably best to take by value - and add a comment about this.

301 ↗(On Diff #30987)

I was wondering WTF was going on there, until I read was isValid does. Needless to say, it doesn't check the validity of the proof id.

This revision now requires changes to proceed.Nov 25 2021, 22:17
Fabien mentioned this in D10556: [avalanche] Simplify the proof verification code in the peer manager.Nov 26 2021, 09:53
deadalnix added inline comments.Nov 26 2021, 14:45
src/avalanche/peermanager.cpp
173 ↗(On Diff #30987)

Move this into createPeer.

264 ↗(On Diff #30987)

Check if the proof is present in the pool, return a tristate: already present, added and conflicted. Make conflicted 0 so that it is falsy.

301 ↗(On Diff #30987)

Do the call addToPool here. If it is added, you failed to add, or it was already present, return.

Fabien mentioned this in rABC1612135f568a: [avalanche] Simplify the proof verification code in the peer manager.Nov 26 2021, 15:19
Fabien mentioned this in D10523: [avalanche] Use the ProofPool multi index to store the orphan proofs.Nov 26 2021, 23:09
Fabien mentioned this in D10525: [avalanche] Make it possible to replace a proof.
Fabien updated this revision to Diff 31119.Nov 27 2021, 00:12
Fabien edited the summary of this revision. (Show Details)
Fabien added a parent revision: D10582: [avalanche] Move the ProofPool definition to its own header.

Make the ProofPool a struct, and replace addToPool/removeFromPool with addProof/removeProof methods.
This removes the need for passing the ProofPool as a parameter, and makes the API cleaner.

src/avalanche/peermanager.cpp
162 ↗(On Diff #30987)

Updated in D10556

301 ↗(On Diff #30987)

Renamed in D10557

Fabien added inline comments.Nov 27 2021, 00:17
src/avalanche/proofpool.h
71 ↗(On Diff #31119)

I don't know if there is a more idiomatic way of doing a tristate in c++ (without pulling boost::tribool). I started with std::optional<bool> but this makes the code very confusing due to the implicit bool conversion provided by std::optional, so I went the naive and verbose way.

Harbormaster completed remote builds in B17424: Diff 31119.Nov 27 2021, 00:31
Fabien updated this revision to Diff 31130.Nov 29 2021, 08:53

Remove unused header

Harbormaster completed remote builds in B17433: Diff 31130.Nov 29 2021, 09:28
Fabien added a child revision: D10586: [avalanche] Add a ProofPool getProof accessor to retrieve a proof from its id.Nov 29 2021, 13:18
deadalnix requested changes to this revision.Nov 29 2021, 22:08
deadalnix added inline comments.
src/avalanche/peermanager.cpp
259 ↗(On Diff #31130)

This if cascade calls for a switch. With no default, it'll warn on missing enums cases.

This revision now requires changes to proceed.Nov 29 2021, 22:08
deadalnix added inline comments.Nov 29 2021, 22:09
src/avalanche/proofpool.cpp
10 ↗(On Diff #31130)

You probably want to assert the proof is not null here.

Fabien updated this revision to Diff 31164.Nov 30 2021, 08:39

Use a switch on the AddProofStatus, and assert the proof is not nullptr in addProof()

Harbormaster completed remote builds in B17456: Diff 31164.Nov 30 2021, 08:58
Fabien removed a child revision: D10523: [avalanche] Use the ProofPool multi index to store the orphan proofs.Nov 30 2021, 10:47
Fabien updated this revision to Diff 31169.Nov 30 2021, 13:03

Invert the proofs sequences in the test. This has no impact on the test actually but will later make it obvious that the behavior changed in D10524.

Harbormaster completed remote builds in B17461: Diff 31169.Nov 30 2021, 13:13
teamcity added a comment.Nov 30 2021, 13: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  | 13 s
wallet_avoidreuse.py                    | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors      | ✓ Passed  | 5 s
wallet_backup.py                        | ✓ Passed  | 25 s
wallet_balance.py                       | ✓ Passed  | 11 s
wallet_basic.py                         | ✓ Passed  | 21 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  | 8 s
wallet_disable.py                       | ✓ Passed  | 1 s
wallet_dump.py                          | ✓ Passed  | 5 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  | 5 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importdescriptors.py             | ✓ Passed  | 7 s
wallet_importmulti.py                   | ✓ Passed  | 4 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  | 2 s
wallet_listreceivedby.py                | ✓ Passed  | 15 s
wallet_listsinceblock.py                | ✓ Passed  | 3 s
wallet_listtransactions.py              | ✓ Passed  | 29 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 11 s
wallet_reorgsrestore.py                 | ✓ Passed  | 4 s
wallet_resendwallettransactions.py      | ✓ Passed  | 1 s
wallet_send.py                          | ✓ Passed  | 8 s
wallet_startup.py                       | ✓ Passed  | 3 s
wallet_txn_clone.py                     | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 5 s
wallet_txn_doublespend.py               | ✓ Passed  | 2 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 5 s
wallet_watchonly.py                     | ✓ Passed  | 1 s
wallet_watchonly.py --usecli            | ✓ Passed  | 1 s

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

----------------------------------------------------------------------
Ran 10 tests in 0.093s

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, 13:45

yet another unrelated failure, restarted the build.

deadalnix accepted this revision.Nov 30 2021, 16:39
deadalnix added inline comments.
src/avalanche/peermanager.cpp
269 ↗(On Diff #31169)

break ? And why braces?

This revision is now accepted and ready to land.Nov 30 2021, 16:39
Closed by commit rABC799f65f976aa: [avalanche] Add wrappers to add and remove a proof from a proof pool (authored by Fabien). · Explain WhyNov 30 2021, 18:57
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC799f65f976aa: [avalanche] Add wrappers to add and remove a proof from a proof pool.
Fabien added inline comments.Nov 30 2021, 19:18
src/avalanche/peermanager.cpp
269 ↗(On Diff #31169)

We have a mix of braces/no braces in the codebase. It seems that braces are added to the most recent code, but it's not consistent.

Revision Contents
Changeset List

PathSize
src/
1 line
avalanche/
57 lines
37 lines
56 lines
test/
1 line
70 lines

Diff 31180

View Options

src/CMakeLists.txt

Loading...
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/CMakeLists.txt

Loading...
View Options

src/avalanche/test/proofpool_tests.cpp

Loading...