[avalanche] add a way to register a proof without a delegation
AbandonedPublic
Actions

Authored by PiRK on May 1 2021, 09:52.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Summary

The current way a proof is registerd is to add a node, which is only possible if we already met a node advertising this proof in a valid delegation.

This adds a way to register a proof into the PeerManager without a delegation, for proofs received via the inventory network messages.
It also adds a getProof method for the network layer to be able to find out if we already know of a proof when receiving an inventory, and to relay that proof to peers if we have it.

The next step is to add a container to be able to register orphans proofs.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

peermanager_use_orphanpool

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15617
Build 31150: Build Diff	lint-circular-dependencies · build-without-wallet · build-debug · build-diff · build-clang-tidy · build-clang
Build 31149: arc lint + arc unit

Event Timeline

PiRK created this revision.May 1 2021, 09:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 1 2021, 09:52

PiRK requested review of this revision.May 1 2021, 09:52

Harbormaster completed remote builds in B15617: Diff 28331.May 1 2021, 10:08

I completely fail to see what this patch achieve. The added code seems to be at best redundant with existing code, at worse introduce bugs (if not today, then later as redundancy gets out of sync).

src/avalanche/peermanager.cpp
184	What is this achieving? I'm pretty sure you can remove this, because isn't it exactly what the first block within `fetchOrCreatePeer` already does?
188	If we make abstraction of the first check, which we deduced is redundant anyways, how does this differs from `getPeerId(proof) != NO_PEER` ?

This revision now requires changes to proceed.May 2 2021, 12:32

PiRK added inline comments.May 3 2021, 06:22

src/avalanche/peermanager.cpp
184	What is this achieving? I'm pretty sure you can remove this, because isn't it exactly what the first block within `fetchOrCreatePeer` already does? My idea is when `getProof` will start also checking for orphan proofs as well, it will no longer be the same. `fetchOrCreatePeer`

In D9469#214913, @deadalnix wrote:

I completely fail to see what this patch achieve. The added code seems to be at best redundant with existing code, at worse introduce bugs (if not today, then later as redundancy gets out of sync).

Currently we only store good proofs in the PeerManager, but the plan is also to store orphan proofs. We need a way to add a proof, have the PeerManager verify it and tell us if it was succesfully added. getPeerId(proof) will only tell us if the proof is good, not if we can store the proof as an orphan.

PiRK edited the summary of this revision. (Show Details)May 3 2021, 08:55

Remove dependency on D9468.
Update the summary to explain what getProof is supposed to be used for.

Make a fetchOrCreatePeer take a reference to a ProofValidationState, so that a caller can know if a proof is permanently bad or is an orphan proof. Update addProof to use this new signature.
The previous implementation of fetchOrCreatePeer create a ProofValidationState and never used it. Maintain an alias for such a case, to avoid having to create a dummy state whenever the caller of fetchOrCreatePeer does not need to know if the proof is an orphan.

Harbormaster completed remote builds in B15621: Diff 28335.May 3 2021, 09:37

Why couldn't fetchOrCreatePeer put the proof in the orphan set based on validation? Why is a new API needed? I strongly suspect that as you actually solve problems that you haven you'll discover that this is not needed.

src/avalanche/peermanager.cpp
184 ↗	(On Diff #28335)	This is literally just the first thing that `fetchOrCreatePeer` does. This is redundant and fragile.

This revision now requires changes to proceed.May 3 2021, 11:56

In D9469#214959, @deadalnix wrote:

Why couldn't fetchOrCreatePeer put the proof in the orphan set based on validation? Why is a new API needed? I strongly suspect that as you actually solve problems that you haven you'll discover that this is not needed.

It is true that the sorting of good vs orphan proofs can be done in fetchOrCreatePeers. But the problem that is currently not solved by the existing API is to get a feedback on why a Peer couldn't be created, to decide whether to put the proof in the recentRejects filter. If I call getPeerId and it returns NO_PEER, I don't know if the proof is permanently bad.
This is why I initially wanted the recentRejects to be handled by the PeerManager in D9423

An alternative to adding a new method is to forward the ProofValidationState all the way up the stack (e.g. PeerId PeerManager::getPeerId(const Proof &proof, ProofValidationState &state)). I did not go down that route because it has the drawback of making the Processor or the network layer have to inspect the reason for the rejection, I think it would be better for this to stay internal to the PeerManager.

Ok, so the problem that you want to solve is knowing if the proof is rejected or just orphaned. The solution can come in many forms. For instance:
1/ Separate validation that doesn't depend from the state (and is therefore def invalid) and validation that is state dependent, put the stateless check outside the peer manager and handle the result there. This doesn't require any changes here.
2/ Do the check here and add a way to export the result, which this patch really doesn't achieve.

Either way, you'll not figure out which is best by trying to guess here. you'll figure out by actually building what's using it. It starts in the network layer.

OK I will try to attack this from the other angle, starting from the network. I have a good idea of the general picture, but the tricky part for me it to split it into smaller units that make sense on their own.

In D9469#214984, @deadalnix wrote:

2/ Do the check here and add a way to export the result, which this patch really doesn't achieve.

This was the plan. The check is already done here. All we need is adding the orphanpool and plug is into addProofand getProof. After that, if addProof returns True, we know that the proof is not bad fore sure, because it has been added either as peers or as an orphan pool.

Solution 1 has the drawback to let many "definitely bad" proofs not be filtered, because most state dependent checks also tell us that the proof is def bad. Only HEIGHT_MISMATCH and MISSING_UTXO can be orphans.

PiRK abandoned this revision.May 31 2021, 07:39