Details

Reviewers

Fabien
deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABC44e68b5da0f6: [avalanche] include proof master and limited proofid in the delegation

Summary

This way, the delegation includes all the data necessary to verify it.
The proof master key is added as a separate member rather than as the first level, because it is a special level that does not need to be signed by anyone else.

Remove the proofid from the serialized delegation, and from the constructor arguments. This can now be computed from the limited id and proof master key. This removes a possible way the delegation can be invalid, so the INCORRECT_PROOF state for the delegation is no longer needed.

Depends on D9596
Depends on D9597

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15774
Build 31448: Build Diff	lint-circular-dependencies · build-clang-tidy · build-without-wallet · build-debug · build-clang · build-diff
Build 31447: arc lint + arc unit

Event Timeline

PiRK created this revision.May 21 2021, 06:49

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 21 2021, 06:49

PiRK requested review of this revision.May 21 2021, 06:49

PiRK added a child revision: D9565: [avalanche] verify signature on avahello reception.May 21 2021, 06:53

PiRK edited the summary of this revision. (Show Details)May 21 2021, 06:58

Harbormaster completed remote builds in B15774: Diff 28561.May 21 2021, 07:01

PiRK mentioned this in D9509: [avalanche] Send the proof master pubkey with AVAHELLO.May 21 2021, 07:24

Fabien requested changes to this revision.May 26 2021, 07:21

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/delegation.cpp
42	This should be a method of ProofId and not Proof (and maybe a constructor)
src/avalanche/delegation.h
39	You can compute the proofid from the limitedid and master rather than supplying it
src/avalanche/delegationbuilder.cpp
66	don't move
src/avalanche/test/delegation_tests.cpp
94	I'm not sure what this does but you're clearly comparing 2 different things

This revision now requires changes to proceed.May 26 2021, 07:21

Fabien added inline comments.May 26 2021, 07:38

src/net_processing.cpp
4010	Remove

Address review. The main change is that the proofid is now no longer part of the serialized Delegation, and no longer needed as a constructor parameter. We can always computed it from the limited id and proof master key. This removes a way for the delegation to be invalid, so I removed the validation state INCORRECT_PROOF.

Move the proof id from limited id computation code to the ProofId class.

Note that in delegation_tests.cpp we remove some data from the hex strings (proofid), but the remaining data (delegation levels) remains unchanged even though the diff will look massive due to shifting the data to the left.

Harbormaster completed remote builds in B15809: Diff 28611.May 26 2021, 11:22

don't use an instance method on a partially constructed object, in the Delegation constructor. It works, but is fragile, it requires a specific order for member initialization.

Call directly ProofId::fromLimitedId and delete computeProofId.

Harbormaster completed remote builds in B15812: Diff 28615.May 26 2021, 13:55

This is doing a number of refactoring in addition to what it is actually doing. You need to split this up.

src/avalanche/delegation.h
25 ↗	(On Diff #28615)	Why?
src/avalanche/proofid.h
9 ↗	(On Diff #28615)	Does anything that uses a ProofId really need to depend on hash and pubkey? Probably not.

deadalnix requested changes to this revision.May 26 2021, 15:47

This revision now requires changes to proceed.May 26 2021, 15:47

rebase onto D9596 and D9597

Don't cache the proofid in Delegation, compute it on demand

Harbormaster completed remote builds in B15833: Diff 28643.May 27 2021, 14:47

deadalnix added inline comments.May 27 2021, 20:16

src/rpc/avalanche.cpp
355 ↗	(On Diff #28643)	How is that possible?

PiRK added inline comments.May 28 2021, 08:01

src/rpc/avalanche.cpp
355 ↗	(On Diff #28643)	AFAIK this is all user supplied data. Serialized Proofs and delegations. So it is definitely possible that a they don't match. Previously the check was done in `dg.verify`, but with this change we no longer supply the proof to `dg.verify`. The delegation is now verified independently of the proof, and the responsibility for checking that they match is moved up in the stack. On the other hand, if a user wants to add a level to a delegation, the proof is now not strictly required any longer. So we could split this RPC into two separate RPCs: `delegateavalancheproof proof privkey pubkey` and `adddelegationlevel privkey pubkey`. The second one would not require any proof, so there would be no need to check..

PiRK added inline comments.May 28 2021, 08:45

src/rpc/avalanche.cpp
355 ↗	(On Diff #28643)	After discussing it with @fabcien, the idea of spliiting it into 2 RPCs does not bring much value to users. A user needs the proof anyway if he wants to use a delegation, so being able to add a delegation level without knowing of the proof is not very useful.

deadalnix requested changes to this revision.May 28 2021, 13:09

deadalnix added inline comments.

src/rpc/avalanche.cpp
355 ↗	(On Diff #28643)	But you don't really need the proof at all to begin with. In fact, it's very inconvenient to have to supply it.

This revision now requires changes to proceed.May 28 2021, 13:09

Do you think we shoud split this into 2 RPCs as I suggested, only one of which would require the proof? Or are you suggesting we should keep it as it is, but only have to provide the necessary data?

Before this diff, we could have refactored DelegationBuilder to provide a proof id and a master key instead of the whole proof, but with this change adding a limited id to the delegation the user would need to specify the limited id and the master key. Is is a good idea to make the limited id a required input for a RPC command? I kind of saw the limited id as an implementation detail that users shouldn't necessarily be aware of, until now.

Mengerian added a task: Restricted Maniphest Task.May 28 2021, 16:31

I'm working on removing the proof parameter from the delegateavalancheproof RPC and replace it with just a limited proof id. After a first attempt at doing this upstream of this commit, I found that it makes more sense to do it just after this change. Having the proofMaster in the delegation makes it possible to access the proof master pubkey without adding another mandatory parameter to the RPC.

So I will submit a separate couple of revisions for it.
Note that it will not remove the check for matching proofids. We can still have a mismatch between the provided limited id and the provided delegation. If we want to remove the check, it is necessary to split this RPC as suggested (delegateproof and adddelegationlevel).

But it gets rid of another verification : no invalid proof.

deadalnix accepted this revision.May 31 2021, 16:07

Fabien accepted this revision.May 31 2021, 19:16

This revision is now accepted and ready to land.May 31 2021, 19:16

Closed by commit rABC44e68b5da0f6: [avalanche] include proof master and limited proofid in the delegation (authored by PiRK). · Explain WhyMay 31 2021, 20:05

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC44e68b5da0f6: [avalanche] include proof master and limited proofid in the delegation.

PiRK mentioned this in D9614: [avalanche] make the delegateavalancheproof RPC take a limited ID.Jun 1 2021, 08:57

[avalanche] include proof master and limited proofid in the delegation
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28561

src/avalanche/delegation.h

src/avalanche/delegation.cpp

src/avalanche/delegationbuilder.h

src/avalanche/delegationbuilder.cpp

src/avalanche/peermanager.cpp

src/avalanche/proof.h

src/avalanche/proof.cpp

src/avalanche/test/delegation_tests.cpp

src/net_processing.cpp

src/rpc/avalanche.cpp

test/functional/abc_rpc_avalancheproof.py

test/functional/test_framework/messages.py

[avalanche] include proof master and limited proofid in the delegationClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28561

src/avalanche/delegation.h

src/avalanche/delegation.cpp

src/avalanche/delegationbuilder.h

src/avalanche/delegationbuilder.cpp

src/avalanche/peermanager.cpp

src/avalanche/proof.h

src/avalanche/proof.cpp

src/avalanche/test/delegation_tests.cpp

src/net_processing.cpp

src/rpc/avalanche.cpp

test/functional/abc_rpc_avalancheproof.py

test/functional/test_framework/messages.py

[avalanche] include proof master and limited proofid in the delegation
ClosedPublic
Actions

Revision Contents
Changeset List