Page MenuHomePhabricator
Differential D10521

[avalanche] Introduce the ProofPool multi index structure
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC98c4acad8c24: [avalanche] Introduce the ProofPool multi index structure
Summary

This diff adds a generic multi index container for storing the proofs, with an utxo based mapping. The proofs can then be retrieve by utxos but also by proofid as needed. This container will be used to replace the orphan pool in a follow-up, hence the name validProofPool by opposition to the coming orphanProofPool. There is no change in behavior.

Ref T1854.

Depends on D10520.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_peermanager_pool_multi_index
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17376
Build 34580: Build Diffbuild-debug · build-without-wallet · build-diff · build-clang · lint-circular-dependencies · build-clang-tidy
Build 34579: arc lint + arc unit

Event Timeline

Fabien created this revision.Nov 24 2021, 16:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 24 2021, 16:14
Fabien requested review of this revision.Nov 24 2021, 16:14
Fabien added a child revision: D10522: [avalanche] Add wrappers to add and remove a proof from a proof pool.Nov 24 2021, 16:24
Harbormaster completed remote builds in B17334: Diff 30986.Nov 24 2021, 16:29
deadalnix requested changes to this revision.Nov 25 2021, 02:23
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/peermanager.h
94 ↗(On Diff #30986)

The outpoint is already in the proof, no need to copy it.

This revision now requires changes to proceed.Nov 25 2021, 02:23
Fabien requested review of this revision.Nov 25 2021, 09:50
Fabien added inline comments.
src/avalanche/peermanager.h
94 ↗(On Diff #30986)

I don't know how to avoid the copy here, if possible at all.
I want a container that use the outpoint as a key, and a proof has outpoints as a 1..n relation so it's not possible to extract an outpoint from a proof so that it is suitable as a key.
Note that it's not making things worst than the actual map which also copies the outpoint key.

Fabien planned changes to this revision.Nov 25 2021, 14:20

So apparently this is possible by using a wrapper function to return the reference, and instruct boost to use it as the source for the key.
Boost store a "pointer to member returning the key" internally, so that member cannot be a reference (pointer to ref), but can be a pointer to a member function that returns a reference.

Fabien updated this revision to Diff 31025.Nov 25 2021, 14:33

Store a const ref to avoid the copy, and use a wrapper function as a key extractor to make boost happy

Harbormaster completed remote builds in B17366: Diff 31025.Nov 25 2021, 14:50
deadalnix added inline comments.Nov 25 2021, 15:53
src/avalanche/peermanager.h
95 ↗(On Diff #31025)

It would be preferable to store an index or a pointer here instead of a reference if this is at all possible, which i think it is due to the accessor.

Fabien updated this revision to Diff 31047.Nov 25 2021, 17:00

Use the index of the utxo as a key

Harbormaster completed remote builds in B17374: Diff 31047.Nov 25 2021, 17:12
deadalnix requested changes to this revision.Nov 25 2021, 18:41
deadalnix added inline comments.
src/avalanche/peermanager.cpp
271 ↗(On Diff #31047)

This clearly doesn't make a lot of sense. The utxo is fetched from the proof, and then there is a very complex process in the other hand to reverse that operation. That is useless work (and possibly a DoS vector is this work is not O(1) ).

src/avalanche/peermanager.h
112 ↗(On Diff #31047)

What is the complexity of this whole contraption?

This revision now requires changes to proceed.Nov 25 2021, 18:41
Fabien updated this revision to Diff 31050.Nov 25 2021, 20:06

Remove the double loop madness. This makes the insert API a bit more tedious but much more efficient.

Harbormaster completed remote builds in B17376: Diff 31050.Nov 25 2021, 20:22
deadalnix accepted this revision.Nov 25 2021, 22:06
deadalnix added inline comments.
src/avalanche/peermanager.cpp
285

Not that if we insert a duplicate, we will delete the original. This is probably a problem.

src/avalanche/peermanager.h
102

I would have put them in the other order, but it's not a blocker.

This revision is now accepted and ready to land.Nov 25 2021, 22:06
Closed by commit rABC98c4acad8c24: [avalanche] Introduce the ProofPool multi index structure (authored by Fabien). · Explain WhyNov 26 2021, 15:24
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC98c4acad8c24: [avalanche] Introduce the ProofPool multi index structure.

Revision Contents
Changeset List

PathSize
src/
avalanche/
42 lines
30 lines
test/
lint/
1 line

Diff 31050

View Options

src/avalanche/peermanager.h

Loading...
View Options

src/avalanche/peermanager.cpp

Loading...
View Options

test/lint/lint-boost-dependencies.sh

Loading...