Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rABC97ecb320bde4: [avalanche] make delegateavalancheproof take a limited id rather than a whole…

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

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jun 1 2021, 08:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 1 2021, 08:55

PiRK requested review of this revision.Jun 1 2021, 08:55

PiRK added a child revision: D9614: [avalanche] make the delegateavalancheproof RPC take a limited ID.Jun 1 2021, 08:57

Harbormaster completed remote builds in B15863: Diff 28693.Jun 1 2021, 09:09

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

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

Harbormaster completed remote builds in B15875: Diff 28705.Jun 1 2021, 16:37

deadalnix requested changes to this revision.Jun 1 2021, 17:52

deadalnix added inline comments.

src/avalanche/delegationbuilder.cpp
23 ↗	(On Diff #28705)	This whole construct seems suspicious. Shouldn't this be a constructor?

This revision now requires changes to proceed.Jun 1 2021, 17:52

PiRK added inline comments.Jun 1 2021, 20:15

src/avalanche/delegationbuilder.cpp
23 ↗	(On Diff #28705)	Yes, it can be made a constructor. Will do.

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.

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

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

Harbormaster completed remote builds in B15900: Diff 28742.Jun 3 2021, 15:22

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

factor the initialization using constructor delegation with a private constructor.

Harbormaster completed remote builds in B15903: Diff 28749.Jun 4 2021, 08:50

deadalnix requested changes to this revision.Jun 4 2021, 10:12

deadalnix added inline comments.

src/avalanche/delegationbuilder.h
10 ↗	(On Diff #28749)
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

use std::make_unique

Harbormaster completed remote builds in B15904: Diff 28750.Jun 4 2021, 11:20

Fabien accepted this revision.Jun 4 2021, 12:08

deadalnix accepted this revision.Jun 4 2021, 14:28

This revision is now accepted and ready to land.Jun 4 2021, 14:28

Closed by commit rABC97ecb320bde4: [avalanche] make delegateavalancheproof take a limited id rather than a whole… (authored by PiRK). · Explain WhyJun 4 2021, 15:57

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC97ecb320bde4: [avalanche] make delegateavalancheproof take a limited id rather than a whole….

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28759

src/avalanche/delegation.h

src/avalanche/delegationbuilder.h

src/avalanche/delegationbuilder.cpp

src/rpc/avalanche.cpp

test/functional/abc_p2p_avalanche.py

test/functional/abc_rpc_avalancheproof.py

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28759

src/avalanche/delegation.h

src/avalanche/delegationbuilder.h

src/avalanche/delegationbuilder.cpp

src/rpc/avalanche.cpp

test/functional/abc_p2p_avalanche.py

test/functional/abc_rpc_avalancheproof.py

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

Revision Contents
Changeset List