[Merged by Bors] - atx: verify syntactic correctness before fetching deps #4814
Conversation
countvonzero
force-pushed
the
verify-before-fetch
branch
from
429e744
to
6bcb372
Compare
|
bors try |
countvonzero
force-pushed
the
verify-before-fetch
branch
from
6bcb372
to
77adfb1
Compare
tryBuild failed: |
Codecov Report
@@ Coverage Diff @@
## develop #4814 +/- ##
=========================================
- Coverage 77.1% 77.1% -0.1%
=========================================
Files 254 254
Lines 30281 30281
=========================================
- Hits 23367 23354 -13
- Misses 5399 5411 +12
- Partials 1515 1516 +1
|
countvonzero
force-pushed
the
verify-before-fetch
branch
from
77adfb1
to
210f2ec
Compare
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
activation/handler.go
Outdated
| if atx.NIPost == nil { | ||
| return fmt.Errorf("nil nipst in gossip for atx %s", atx.ShortString()) | ||
| if err := h.SyntacticallyValidate(ctx, &atx); err != nil { | ||
| return err |
There was a problem hiding this comment.
How about adding little context like for ...ValidateDeps()?
| return err | |
| return fmt.Errorf("atx %v syntactically invalid: %w", atx.ShortString(), err) |
There was a problem hiding this comment.
honestly i cannot think of a good context to add to the name. suggestions?
activation/validation.go
Outdated
| if challenge.CommitmentATX == nil { | ||
| return fmt.Errorf("no prevATX declared, but commitmentATX is missing") | ||
| v.log.Panic("commitment atx is nil") |
There was a problem hiding this comment.
Why panic here? It opens a door to crash the node with a malicious ATX.
There was a problem hiding this comment.
to check programming error. it's already verified that in SyntacticallyValidate() that this cannot be nil. but anyway. changed to return error
There was a problem hiding this comment.
the idea was that all the nil were checked in the first stage of syntactic check. here it's doing checks on the deps. but not a big deal.
activation/validation.go
Outdated
| if *id == types.EmptyATXID { | ||
| return fmt.Errorf("empty positioning atx") | ||
| v.log.Panic("empty positioning atx") |
There was a problem hiding this comment.
ditto.
Btw, if id cannot be nil, then perhaps it should be passed by value to avoid the possibility of dereferencing a nil at all? The compiler should be able to optimize the copy away anyway.
There was a problem hiding this comment.
changed. thanks
|
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
countvonzero
force-pushed
the
verify-before-fetch
branch
from
256899a
to
cf58497
Compare
|
bors merge |
## Motivation part of #4632 ## Changes break syntactic checks into 3 steps: - do all the syntactically correctness checks on the current atx without its dependencies - fetching atx dependencies, including poet proof, prev/positioning/commitment atxs - do the syntactic checks based on the referenced atxs.
|
Build failed: |
|
bors merge |
## Motivation part of #4632 ## Changes break syntactic checks into 3 steps: - do all the syntactically correctness checks on the current atx without its dependencies - fetching atx dependencies, including poet proof, prev/positioning/commitment atxs - do the syntactic checks based on the referenced atxs.
|
Pull request successfully merged into develop. Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
bors
bot
changed the title
Motivation
part of #4632
Changes
break syntactic checks into 3 steps: