Page MenuHomePhabricator

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

Authored by PiRK on May 21 2021, 06:49.

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.May 21 2021, 06:49
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 ↗(On Diff #28561)

This should be a method of ProofId and not Proof (and maybe a constructor)

src/avalanche/delegation.h
39 ↗(On Diff #28561)

You can compute the proofid from the limitedid and master rather than supplying it

src/avalanche/delegationbuilder.cpp
66 ↗(On Diff #28561)

don't move

src/avalanche/test/delegation_tests.cpp
94 ↗(On Diff #28561)

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
src/net_processing.cpp
4010 ↗(On Diff #28561)

Remove

PiRK edited the summary of this revision. (Show Details)

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.

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.

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
PiRK edited the summary of this revision. (Show Details)

rebase onto D9596 and D9597

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

src/rpc/avalanche.cpp
355 ↗(On Diff #28643)

How is that possible?

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..

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
PiRK requested review of this revision.EditedMay 31 2021, 16:06

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.

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