Details

Reviewers

PiRK
deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABCe5c8cc0be46d: [avalanche] Sign the avalanche proof

Summary

In order to make it posible to update some parameters of a proof without having to sign the stakes again, we need to reduce the scope of what is signed by the stake. This means that we need to compensate this lack of stake signature score, and this diff creates a new proof signature to achieve that. The proof signs the limited proofid with the key corresponding to the master key, so that no one but the master key owner can update the proof without having the stake to sign again.

As a consequence to the master key change, there is one test case (Changing the master key affect ProofId) which is no longer relevant, as changing the key will raise a proof signature error and no longer a stake signature error, and ends up duplicated. This test case has been removed.

Ref T1676.

Depends on D10300.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_proof_signature

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16958
Build 33752: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 33751: arc lint + arc unit

Event Timeline

Fabien created this revision.Sep 16 2021, 20:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 16 2021, 20:10

Fabien requested review of this revision.Sep 16 2021, 20:10

Harbormaster completed remote builds in B16682: Diff 30012.Sep 16 2021, 20:25

PiRK accepted this revision.Sep 17 2021, 15:35

This revision is now accepted and ready to land.Sep 17 2021, 15:35

Always sign the proof we are building. If the format is legacy then it will just be unused.

Harbormaster completed remote builds in B16743: Diff 30097.Sep 21 2021, 13:27

Fabien added a child revision: D10175: [avalanche] Deduplicate proof id computation.Sep 22 2021, 08:26

deadalnix requested changes to this revision.Sep 22 2021, 18:29

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/avalanche/test/proof_tests.cpp
313 ↗	(On Diff #30097)	Don't remove th test for the format that is deployed now.

This revision now requires changes to proceed.Sep 22 2021, 18:29

Fabien requested review of this revision.Sep 22 2021, 18:37

Fabien added inline comments.

src/avalanche/test/proof_tests.cpp
313 ↗	(On Diff #30097)	They're not removed, they're just above. Both cases (legacy and regular) are tested.

Rebase

Fabien edited the summary of this revision. (Show Details)Sep 23 2021, 09:47

Fabien added a parent revision: D10179: [avalanche] Introduce the StakeCommitment class.

Fabien removed a parent revision: D10109: [avalanche] Add a payout script to the proof.

Fabien added a child revision: D10151: [avalanche] Only sign the subset of data we want to commit the stakes to.Sep 23 2021, 10:09

Harbormaster completed remote builds in B16765: Diff 30141.Sep 23 2021, 10:18

deadalnix requested changes to this revision.Sep 27 2021, 22:08

deadalnix added inline comments.

src/avalanche/test/proof_tests.cpp
314 ↗	(On Diff #30141)	You need to keep the legacy test vectors around to make sure that none of this is fubaring them.

This revision now requires changes to proceed.Sep 27 2021, 22:08

Rebase on top of D10216 and expand upon what changed in the tests in the summary.
Hopefully this will make more explicit what are the changes to this hex mess, and
give confidence that the tests are correct and covering the relevant cases.

Fabien edited the summary of this revision. (Show Details)Sep 29 2021, 19:26

Fabien added a parent revision: D10216: [avalanche] Make regular proof format deserialization tests in par with the legacy.

Fabien removed a parent revision: D10179: [avalanche] Introduce the StakeCommitment class.

Harbormaster completed remote builds in B16816: Diff 30218.Sep 29 2021, 19:36

Fabien mentioned this in D10222: [avalanche] Let the peermanager select whether a conflicting proof should be preferred.Sep 30 2021, 11:50

Fabien planned changes to this revision.Sep 30 2021, 15:12

Fix the legacy test vectors clones that had their payout address updated by mistake

Harbormaster completed remote builds in B16842: Diff 30248.Sep 30 2021, 19:07

deadalnix requested changes to this revision.Oct 1 2021, 14:29

deadalnix added inline comments.

src/avalanche/test/proof_tests.cpp
324 ↗	(On Diff #30248)	The proof are completely different because they are generated with the new key? If so, the key change should be in its own patch, so we can check that the changes made here are sensible. If everything changes, there is nothing to review.