Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace wasIssued with shouldIssueBlock #3131

Merged
merged 18 commits into from
Jun 27, 2024
Merged

Replace wasIssued with shouldIssueBlock #3131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fb43108
Replace wasIssued
StephenButtolph Jun 19, 2024
202cc2e
backport
StephenButtolph Jun 19, 2024
ad05b22
Add regression test
StephenButtolph Jun 20, 2024
8793de4
Fix regression test to pass on master
StephenButtolph Jun 20, 2024
d023b41
nit
StephenButtolph Jun 20, 2024
3595874
Introduce canDependOn
StephenButtolph Jun 20, 2024
14b76b7
nit
StephenButtolph Jun 20, 2024
3f6bba4
lint
StephenButtolph Jun 20, 2024
29ce725
add todo
StephenButtolph Jun 20, 2024
ca3fd85
merged
StephenButtolph Jun 20, 2024
f3987cc
Merge branch 'master' into simplify-dependency-registration
StephenButtolph Jun 20, 2024
61bb415
fix merge
StephenButtolph Jun 20, 2024
ca5560e
Merge branch 'simplify-dependency-registration' into improve-was-issued
StephenButtolph Jun 20, 2024
0fdca3e
nit comment
StephenButtolph Jun 24, 2024
e7caabe
nit
StephenButtolph Jun 25, 2024
46cce68
Merge branch 'master' into simplify-dependency-registration
StephenButtolph Jun 25, 2024
13d756a
Merge branch 'simplify-dependency-registration' into improve-was-issued
StephenButtolph Jun 25, 2024
e4ba298
merged
StephenButtolph Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions snow/engine/snowman/transitive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func (t *Transitive) Put(ctx context.Context, nodeID ids.NodeID, requestID uint3
issuedMetric = t.metrics.issued.WithLabelValues(unknownSource)
}

if t.wasIssued(blk) {
if !t.shouldIssueBlock(blk) {
t.metrics.numUselessPutBytes.Add(float64(len(blkBytes)))
}

Expand Down Expand Up @@ -335,7 +335,7 @@ func (t *Transitive) PushQuery(ctx context.Context, nodeID ids.NodeID, requestID
return nil
}

if t.wasIssued(blk) {
if !t.shouldIssueBlock(blk) {
t.metrics.numUselessPushQueryBytes.Add(float64(len(blkBytes)))
}

Expand Down Expand Up @@ -737,7 +737,7 @@ func (t *Transitive) issueFrom(
) error {
// issue [blk] and its ancestors to consensus.
blkID := blk.ID()
for !t.wasIssued(blk) {
for t.shouldIssueBlock(blk) {
StephenButtolph marked this conversation as resolved.
Show resolved Hide resolved
err := t.issue(ctx, nodeID, blk, false, issuedMetric)
if err != nil {
return err
Expand Down Expand Up @@ -775,7 +775,7 @@ func (t *Transitive) issueWithAncestors(
) error {
blkID := blk.ID()
// issue [blk] and its ancestors into consensus
for !t.wasIssued(blk) {
for t.shouldIssueBlock(blk) {
err := t.issue(ctx, t.Ctx.NodeID, blk, true, issuedMetric)
if err != nil {
return err
Expand All @@ -798,14 +798,6 @@ func (t *Transitive) issueWithAncestors(
return t.blocked.Abandon(ctx, blkID)
}

// If the block has been decided, then it is marked as having been issued.
// If the block is processing, then it was issued.
// If the block is queued to be added to consensus, then it was issued.
func (t *Transitive) wasIssued(blk snowman.Block) bool {
blkID := blk.ID()
return t.isDecided(blk) || t.Consensus.Processing(blkID) || t.pendingContains(blkID)
}

// Issue [blk] to consensus once its ancestors have been issued.
// If [push] is true, a push query will be used. Otherwise, a pull query will be
// used.
Expand Down Expand Up @@ -1044,12 +1036,6 @@ func (t *Transitive) deliver(
return nil
}

// Returns true if the block whose ID is [blkID] is waiting to be issued to consensus
func (t *Transitive) pendingContains(blkID ids.ID) bool {
_, ok := t.pending[blkID]
return ok
}

func (t *Transitive) addToNonVerifieds(blk snowman.Block) {
// don't add this blk if it's decided or processing.
blkID := blk.ID()
Expand Down Expand Up @@ -1159,6 +1145,30 @@ func (t *Transitive) getProcessingAncestor(ctx context.Context, initialVote ids.
}
}

// shouldIssueBlock returns true if the provided block should be enqueued for
// issuance. If the block is already decided, already enqueued, or has already
// been issued, this function will return false.
func (t *Transitive) shouldIssueBlock(blk snowman.Block) bool {
Copy link
Contributor

@marun marun Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No action required) Might this be easier to test exhaustively were it converted to a pure function whose inputs could be trivially varied without regard to chain structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you recommend the signature to look like? We could take in the Consensus interface and the pending map, but I feel like I'd define the test essentially the same way as it is currently structured...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pure function could take the interface and map, or just the logical results of use of those types, or anonymous functions that wrap access. Here is the 'logical results of use' approach, primarily for illustrative purposes:

func (t *Transitive) shouldIssueBlock(blk snowman.Block) bool {
  blkID := blk.ID()
  height := blk.Height()
  lastAcceptedID, lastAcceptedHeight := t.Consensus.LastAccepted()
  _, isPending := t.pending[blkID]
  return shouldIssueBlock(height, lastAcceptedID, lastAcceptedHeight, blk.Parent(), height-1, isPending, t.Consensus.Processing(blkID))  
}

func shouldIssueBlock(height uint64, lastAcceptedID ids.ID, lastAcceptedHeight uint64, parentID ids.ID, parentHeight uint64, isPending bool, isProcessing bool) bool

Not that this is always the best way to approach the problem, but for anything complicated to verify it can be a useful exercise.

height := blk.Height()
lastAcceptedID, lastAcceptedHeight := t.Consensus.LastAccepted()
if height <= lastAcceptedHeight {
return false // block is either accepted or rejected
}

// This is guaranteed not to underflow because the above check ensures
marun marked this conversation as resolved.
Show resolved Hide resolved
// [height] > 0.
parentHeight := height - 1
parentID := blk.Parent()
if parentHeight == lastAcceptedHeight && parentID != lastAcceptedID {
return false // the parent was rejected
}

blkID := blk.ID()
_, isPending := t.pending[blkID]
return !isPending && // If the block is already pending, don't issue it again.
!t.Consensus.Processing(blkID) // If the block was previously issued, don't issue it again.
}

// canDependOn reports true if it is guaranteed for the provided block ID to
// eventually either be fulfilled or abandoned.
func (t *Transitive) canDependOn(blkID ids.ID) bool {
Expand Down
Loading
Loading