[Merged by Bors] - AtxBuilder verification of initial PoST #6031
Conversation
There was a problem hiding this comment.
Overall looks good to me so far. 🙂
I would just also drop the publishEpoch parameter for BuildNiPost (since it is part of the challenge anyway) and maybe add a test to nipost_test.go that checks that a new initial post is generated when the existing initial post fails validation.
Right now the CI fails because the packages activation/e2e, node and systest need to be updated to the new interface of the nipostBuilder. In all cases it should just be passing a (already existing instance of the post verifier) to the NewNIPostBuilder function.
| @@ -53,9 +53,10 @@ type nipostBuilder interface { | |||
| ctx context.Context, | |||
| sig *signing.EdSigner, | |||
| publish types.EpochID, | |||
| challenge types.Hash32, | |||
| challengeHash types.Hash32, | |||
| postChallenge *types.NIPostChallenge, | |||
There was a problem hiding this comment.
NIT: technically its a poet challenge. The hash of it is sent to the poet service. It collects the data that needs to be proven that was selected and known before registering to the poet.
The challenge for PoST is the hash of the poet proof.
| postChallenge *types.NIPostChallenge, | |
| nipostChallenge *types.NIPostChallenge, |
There was a problem hiding this comment.
Not sure if I fully understand, if it is a poet challenge then why are you suggesting to rename it to nipostChallenge? and is there a semantic difference between a postChallenge and a nipostChallenge?
There was a problem hiding this comment.
The hash of the nipost challenge is the one submitted to the poet (that's why I said "poet challenge", a "challenge for the poet").
is there a semantic difference between a postChallenge and a nipostChallenge?
The naming is confusing, we have too many "challenges". The simplified flow is:
- build nipost challenge
- submit hash of nipost challenge to the poet
- wait 2w, obtain the poet proof
- use the root of the poet proof as the challenge to generate the PoST ("post challenge"):
go-spacemesh/activation/nipost.go
Line 290 in 7b09993
acud
force-pushed
the
atx-initial-post-check
branch
from
8d60b7b to
f11e652
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6031 +/- ##
=========================================
- Coverage 81.3% 81.3% -0.1%
=========================================
Files 296 296
Lines 31635 31667 +32
=========================================
+ Hits 25741 25766 +25
+ Misses 4213 4212 -1
- Partials 1681 1689 +8 ☔ View full report in Codecov by Sentry. |
acud
force-pushed
the
atx-initial-post-check
branch
from
db361d1 to
35fb662
Compare
|
bors try |
|
🔒 Permission denied Existing reviewers: click here to make acud a reviewer |
|
bors try |
|
🔒 Permission denied Existing reviewers: click here to make acud a reviewer |
|
So marked this as ready for review. I think it's good for a review now. |
acud
force-pushed
the
atx-initial-post-check
branch
from
a1c2990 to
4966611
Compare
|
bors try |
tryBuild succeeded: |
|
We should also add this fix to the |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild succeeded: |
acud
force-pushed
the
atx-initial-post-check
branch
from
435b11c to
6b80265
Compare
|
bors try |
|
bors cancel |
|
bors merge |
## Motivation In #5921, an edge case is described where a node may start with a certain amount of SUs, generate an initial PoST but for possible reasons, the number of SUs may change before the first ATX is broadcasted. This means that the initial PoST is no longer valid and needs to be regenerated (costing the miner another epoch before being able to submit the initial ATX).
|
Build failed: |
tryBuild failed: |
|
bors merge |
## Motivation In #5921, an edge case is described where a node may start with a certain amount of SUs, generate an initial PoST but for possible reasons, the number of SUs may change before the first ATX is broadcasted. This means that the initial PoST is no longer valid and needs to be regenerated (costing the miner another epoch before being able to submit the initial ATX).
|
Pull request successfully merged into develop. Build succeeded: |
spacemesh-bors
bot
changed the title
Motivation
In #5921, an edge case is described where a node may start with a certain amount of SUs, generate an initial PoST but for possible reasons, the number of SUs may change before the first ATX is broadcasted. This means that the initial PoST is no longer valid and needs to be regenerated (costing the miner another epoch before being able to submit the initial ATX).
Description
This PR fixes the issue by adding an extra check that verifies that the rust process has the same parameters (SUs among them) by executing a proof validation for the existing cached proof with the parameters that are used by the rust process (therefore, if the rust service is restarted with different parameters, this will be caught by this changeset).
Closes #5921
Test Plan
Unit test coverage in
activation_test.goandnipost_test.goTODO
nipost_test.go