Page MenuHomePhabricator

[avalanche] Sign the avalanche proof
ClosedPublic

Authored by Fabien on Sep 16 2021, 20:10.

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

Event Timeline

Fabien requested review of this revision.Sep 16 2021, 20:10
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.

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.

deadalnix requested changes to this revision.Mon, Sep 27, 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.Mon, Sep 27, 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 planned changes to this revision.Thu, Sep 30, 15:12

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

deadalnix requested changes to this revision.Fri, Oct 1, 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.

This revision now requires changes to proceed.Fri, Oct 1, 14:29

Make the test changes obvious by rebasing over D10300.

This revision is now accepted and ready to land.Wed, Oct 13, 21:14
This revision was automatically updated to reflect the committed changes.