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

[Merged by Bors] - Optimize searching for positioning ATX #5952

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Optimize searching for positioning ATX #5952

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d77f35
Use atxsdata to search for a positioning ATX
poszu May 17, 2024
7a599e7
Update activation/activation.go
poszu May 20, 2024
ca7c7b1
Extend E2E ATX builder test to verify positoning ATXs
poszu May 20, 2024
32b4a96
Remove redundant 'filters' argument
poszu May 21, 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
132 changes: 83 additions & 49 deletions activation/activation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

"github.com/spacemeshos/go-spacemesh/activation/metrics"
"github.com/spacemeshos/go-spacemesh/activation/wire"
"github.com/spacemeshos/go-spacemesh/atxsdata"
"github.com/spacemeshos/go-spacemesh/codec"
"github.com/spacemeshos/go-spacemesh/common/types"
"github.com/spacemeshos/go-spacemesh/events"
Expand Down Expand Up @@ -72,6 +73,7 @@
coinbaseAccount types.Address
conf Config
db sql.Executor
atxsdata *atxsdata.Data
localDB *localsql.Database
publisher pubsub.Publisher
nipostBuilder nipostBuilder
Expand Down Expand Up @@ -152,6 +154,7 @@
func NewBuilder(
conf Config,
db sql.Executor,
atxsdata *atxsdata.Data,
localDB *localsql.Database,
publisher pubsub.Publisher,
nipostBuilder nipostBuilder,
Expand All @@ -165,6 +168,7 @@
signers: make(map[types.NodeID]*signing.EdSigner),
conf: conf,
db: db,
atxsdata: atxsdata,
localDB: localDB,
publisher: publisher,
nipostBuilder: nipostBuilder,
Expand Down Expand Up @@ -507,12 +511,6 @@
}
}

posAtx, err := b.getPositioningAtx(ctx, nodeID, publish)
if err != nil {
return nil, fmt.Errorf("failed to get positioning ATX: %w", err)
}
logger.Info("selected positioning atx", log.ZShortStringer("atx_id", posAtx))

prevAtx, err = b.GetPrevAtx(nodeID)
switch {
case errors.Is(err, sql.ErrNotFound):
Expand All @@ -538,6 +536,10 @@
}
return nil, fmt.Errorf("initial POST is invalid: %w", err)
}
posAtx, err := b.getPositioningAtx(ctx, nodeID, publish, nil)
if err != nil {
return nil, fmt.Errorf("failed to get positioning ATX: %w", err)

Check warning on line 541 in activation/activation.go

View check run for this annotation

Codecov / codecov/patch

activation/activation.go#L541 

Added line #L541 was not covered by tests
}
challenge = &types.NIPostChallenge{
PublishEpoch: publish,
Sequence: 0,
Expand All @@ -554,6 +556,10 @@
return nil, fmt.Errorf("get last ATX: %w", err)
default:
// regular ATX challenge
posAtx, err := b.getPositioningAtx(ctx, nodeID, publish, prevAtx)
if err != nil {
return nil, fmt.Errorf("failed to get positioning ATX: %w", err)

Check warning on line 561 in activation/activation.go

View check run for this annotation

Codecov / codecov/patch

activation/activation.go#L561 

Added line #L561 was not covered by tests
}
challenge = &types.NIPostChallenge{
PublishEpoch: publish,
Sequence: prevAtx.Sequence + 1,
Expand Down Expand Up @@ -692,6 +698,12 @@
break
}
if nipostState.VRFNonce != oldNonce {
b.log.Info(
"attaching a new VRF nonce in ATX",
log.ZShortStringer("smesherID", sig.NodeID()),
zap.Uint64("new nonce", uint64(nipostState.VRFNonce)),
zap.Uint64("old nonce", uint64(oldNonce)),
)
nonce = &nipostState.VRFNonce
}
}
Expand Down Expand Up @@ -723,9 +735,9 @@
return len(buf), nil
}

// getPositioningAtx returns atx id with the highest tick height.
// searchPositioningAtx returns atx id with the highest tick height.
// publish epoch is used for caching the positioning atx.
func (b *Builder) getPositioningAtx(
func (b *Builder) searchPositioningAtx(
ctx context.Context,
nodeID types.NodeID,
publish types.EpochID,
Expand All @@ -738,34 +750,65 @@
return found.id, nil
}

logger.Info("searching for positioning atx")
latestPublished, err := atxs.LatestEpoch(b.db)
if err != nil {
return types.EmptyATXID, fmt.Errorf("get latest epoch: %w", err)
}
logger.Info("searching for positioning atx", zap.Uint32("latest_epoch", latestPublished.Uint32()))
// positioning ATX publish epoch must be lower than the publish epoch of built ATX
positioningAtxPublished := min(latestPublished, publish-1)
id, err := findFullyValidHighTickAtx(
ctx,
b.db,
nodeID,
b.atxsdata,
positioningAtxPublished,
b.conf.GoldenATXID,
b.validator,
logger,
VerifyChainOpts.AssumeValidBefore(time.Now().Add(-b.postValidityDelay)),
VerifyChainOpts.WithTrustedID(nodeID),
VerifyChainOpts.WithLogger(b.log),
)
switch {
case err == nil:
b.posAtxFinder.found = &struct {
id types.ATXID
forPublish types.EpochID
}{id, publish}
return id, nil
case errors.Is(err, sql.ErrNotFound):
logger.Info("using golden atx as positioning atx")
b.posAtxFinder.found = &struct {
id types.ATXID
forPublish types.EpochID
}{b.conf.GoldenATXID, publish}
return b.conf.GoldenATXID, nil
if err != nil {
logger.Info("search failed - using golden atx as positioning atx", zap.Error(err))
id = b.conf.GoldenATXID
}
return id, err
b.posAtxFinder.found = &struct {
id types.ATXID
forPublish types.EpochID
}{id, publish}

return id, nil
}

// getPositioningAtx returns the positioning ATX.
// The provided previous ATX is picked if it has a greater or equal
// tick count as the ATX selected in `searchPositioningAtx`.
func (b *Builder) getPositioningAtx(
ctx context.Context,
nodeID types.NodeID,
publish types.EpochID,
previous *types.ActivationTx,
) (types.ATXID, error) {
id, err := b.searchPositioningAtx(ctx, nodeID, publish)
if err != nil {
return types.EmptyATXID, err
}

if previous != nil {
switch {
case id == b.conf.GoldenATXID:
id = previous.ID()
case id != b.conf.GoldenATXID:
if candidate, err := atxs.Get(b.db, id); err == nil {
if previous.TickHeight() >= candidate.TickHeight() {
id = previous.ID()
}
}
}
Comment on lines +801 to +807
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this check is necessary? If we have a previous ATX and the search found something then this will always be the previous ATX or one with a higher TickHeight, won't it be? I get the intention of preferring one's own ATX in the case that the candidate is a different ATX with the exact same TickHeight but I don't think that this is strictly advantageous? The candidate has already been verified to be valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The search in atxsdata doesn't prefer own ATXs - it picks the first of the highest randomly. The check here ensures we pick our own ATX.

I get the intention of preferring one's own ATX in the case that the candidate is a different ATX with the exact same TickHeight but I don't think that this is strictly advantageous?

The only advantage I see is that it's "less risky" to refer to something created by self. The existing code does the same and I wanted to keep the functionality unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

The check here ensures we pick our own ATX.

only in the case of a tie. Even if the node decided candidate is valid although it isn't this has no impact on ATXs that use it as a positioning ATX. As long as it is syntactically valid it can be referenced even if malfeasant. It has to be like this or marking an identity as malfeasant would cause everyone who referenced them (directly or indirectly) to fail validation of their published ATXs.

I'm not blocking the merge of this PR because of this, just making sure we are on the same page 🙂

}

b.log.Info("selected positioning atx", log.ZShortStringer("id", id), log.ZShortStringer("smesherID", nodeID))
return id, nil
}

func (b *Builder) Regossip(ctx context.Context, nodeID types.NodeID) error {
Expand Down Expand Up @@ -797,35 +840,26 @@

func findFullyValidHighTickAtx(
ctx context.Context,
db sql.Executor,
prefNodeID types.NodeID,
atxdata *atxsdata.Data,
publish types.EpochID,
goldenATXID types.ATXID,
validator nipostValidator,
logger *zap.Logger,
opts ...VerifyChainOption,
) (types.ATXID, error) {
rejectedAtxs := make(map[types.ATXID]struct{})
filter := func(id types.ATXID) bool {
_, ok := rejectedAtxs[id]
return !ok
}

for {
select {
case <-ctx.Done():
return types.ATXID{}, ctx.Err()
default:
}
id, err := atxs.GetIDWithMaxHeight(db, prefNodeID, filter)
if err != nil {
return types.ATXID{}, err
}
logger.Info("found candidate for high-tick atx, verifying its chain", log.ZShortStringer("atx_id", id))
var found *types.ATXID
atxdata.IterateHighTicksInEpoch(publish+1, func(id types.ATXID) bool {
logger.Info("found candidate for high-tick atx", log.ZShortStringer("id", id))
if err := validator.VerifyChain(ctx, id, goldenATXID, opts...); err != nil {
logger.Info("rejecting candidate for high-tick atx", zap.Error(err), log.ZShortStringer("atx_id", id))
rejectedAtxs[id] = struct{}{}
} else {
return id, nil
logger.Info("rejecting candidate for high-tick atx", zap.Error(err), log.ZShortStringer("id", id))
return true
}
found = &id
return false
}, atxsdata.NotMalicious)

if found != nil {
return *found, nil
}
return types.ATXID{}, ErrNotFound
}
Loading