Page MenuHomePhabricator

[avalanche] make delegateavalancheproof take a limited id rather than a whole proof
ClosedPublic

Authored by PiRK on Jun 1 2021, 08:55.

Details

Summary

The DelegationBuilder does not need a complete proof to build a delegation, only its master public key and its limited proof id. Add a constructor that does not require the proof.
Remove also the proofid member that can be computed from the limited ID.

Replace the DelegationBuilder::importDelegation method with a DelegationBuilder(const Delegation &dg) constructor.

Use these 2 new constructors in the delegateavalancheproof RPC.

Use a private constructor to factor the common code for the 3 public constructors.

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.Jun 1 2021, 08:55
deadalnix requested changes to this revision.Jun 1 2021, 15:37
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/test/delegation_tests.cpp
38 ↗(On Diff #28693)

You can have two constructor instead of changing all callsites.

This revision now requires changes to proceed.Jun 1 2021, 15:37
PiRK edited the summary of this revision. (Show Details)

Don't remove the constructor that takes a proof, so that callsites don't need changing.

deadalnix requested changes to this revision.Jun 1 2021, 17:52
deadalnix added inline comments.
src/avalanche/delegationbuilder.cpp
23

This whole construct seems suspicious. Shouldn't this be a constructor?

This revision now requires changes to proceed.Jun 1 2021, 17:52
src/avalanche/delegationbuilder.cpp
23

Yes, it can be made a constructor. Will do.

PiRK retitled this revision from [avalanche] make DelegationBuilder constructor take a limited ID and master key to [avalanche] make delegateavalancheproof take a limited id rather than a whole proof.
PiRK edited the summary of this revision. (Show Details)
PiRK edited the test plan for this revision. (Show Details)

Replace DelegationBuilder::importDelegation with a constructor. This forces me to modify delegateavalancheproof immediately, so merge this revision with D9614.

The two branches of delegateavalancheproof now require two different DelegationBuilder constructors. I don't see how this can be done without using a dgb pointer or duplicating a lot of code.

Fabien requested changes to this revision.Jun 3 2021, 08:21
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/avalanche/delegationbuilder.cpp
20 ↗(On Diff #28714)

You can use a delegated constructor to avoid the boilerplate

src/avalanche/delegationbuilder.h
18 ↗(On Diff #28714)

Do it the other way, use forwarding when possible

27 ↗(On Diff #28714)

explicit is not needed here

test/functional/abc_rpc_avalancheproof.py
242 ↗(On Diff #28714)

Nit: move with other proof constructions

This revision now requires changes to proceed.Jun 3 2021, 08:21

address review:

  • delegate constructor for DelegationBuilder(const Proof &p)
  • use forward declarations in the header file when possible
  • move a buildavalancheproof RPC call in the functional test
Fabien requested changes to this revision.Jun 3 2021, 16:16
Fabien added inline comments.
src/avalanche/delegationbuilder.cpp
25 ↗(On Diff #28742)

If you start from a private DelegationBuilder(const LimitedProofId &limitedProofIdIn, DelegationId dgidIn, const CPubKey &proofMaster) you can have them all without repeating the levels.push_back({proofMaster, {}})

This revision now requires changes to proceed.Jun 3 2021, 16:16
PiRK edited the summary of this revision. (Show Details)

factor the initialization using constructor delegation with a private constructor.

deadalnix requested changes to this revision.Jun 4 2021, 10:12
deadalnix added inline comments.
src/avalanche/delegationbuilder.h
10 ↗(On Diff #28749)

likestamp

src/rpc/avalanche.cpp
338 ↗(On Diff #28749)

See, this makes the code simpler.

368 ↗(On Diff #28749)

Use make_unique instead of reset.

371 ↗(On Diff #28749)

dito

test/functional/abc_rpc_avalancheproof.py
178 ↗(On Diff #28749)

And it makes the test simpler.

This revision now requires changes to proceed.Jun 4 2021, 10:12
This revision is now accepted and ready to land.Jun 4 2021, 14:28