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.

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

Allow for querying proofs and remove broadcasting #187

Merged
merged 16 commits into from
Dec 16, 2022

Conversation

poszu
Copy link
Contributor

@poszu poszu commented Dec 12, 2022

Implements necessary poet changes for spacemeshos/pm#173

Closes #167
Closes #168
Closes #169

Notable changes:

  • Service reports produced proofs via a channel instead of broadcasting them. The proofs are then picked up by consumers:

    • in a poet server case, the consumer is a database actor that serializes and persists them
    • in tests, the test is a consumer directly
  • removed the last Mutex from the Service struct. The actor event loop is now synchonous (started via Service::Run()). It doesn't spawn a goroutine and finishes when the context is cancelled. It made the shutdown logic redundant.

  • split server startup into:

    • creation: server::New(), where it configures itself and binds ports to listen on
    • start: Server::Start, where it actually starts to process incoming requests

    It allows writing tests that run on random ports and obtain ports the server listens on, which lets run parallel tests.

  • Submit() GRPC returns duration till the end of the round that the challenge was submitted to. It gives two benefits for the node:

    • it can easily verify if the resulting proof will be valuable for it (if it ends on time). It might not be if it missed the registration window,
    • it knows when to query for the proof

@poszu poszu self-assigned this Dec 12, 2022
Copy link
Contributor

@countvonzero countvonzero left a comment

Choose a reason for hiding this comment

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

some high level questions first to get the context. thanks for your patience.

rpc/api/api.proto Outdated Show resolved Hide resolved
shared/shared.go Outdated Show resolved Hide resolved
service/db.go Show resolved Hide resolved
service/db.go Outdated Show resolved Hide resolved
service/db.go Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
service/service.go Outdated Show resolved Hide resolved
@poszu poszu requested a review from countvonzero December 15, 2022 09:32
@poszu poszu force-pushed the pm-173-remove-proof-broadcasting branch from 7a9ffd8 to 7a9301c Compare December 15, 2022 10:37
// GetProof implements api.PoetServer.
func (r *rpcServer) GetProof(ctx context.Context, in *api.GetProofRequest) (*api.GetProofResponse, error) {
if info, err := r.s.Info(ctx); err == nil {
if info.OpenRoundID == in.RoundId || slices.Contains(info.ExecutingRoundsIds, in.RoundId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i still finds this multiple rounds logic confusing. would appreciate (for future reviews) that we get rid of it if we know poet is not doing multiple rounds at the same time.

proof, err := r.proofsDb.Get(ctx, in.RoundId)
switch {
case errors.Is(err, service.ErrNotFound):
return nil, status.Error(codes.NotFound, "proof not found")
Copy link
Contributor

Choose a reason for hiding this comment

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

would printing err.Error() here be more informative for debugging?

Copy link
Contributor Author

@poszu poszu Dec 16, 2022

Choose a reason for hiding this comment

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

The GRPC interceptor already prints requests and responses here 😉:

poet/server/server.go

Lines 174 to 191 in c2c2e82

func loggerInterceptor() func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
peer, _ := peer.FromContext(ctx)
logger := log.AppLog.WithName(info.FullMethod).WithFields(log.Field(zap.Stringer("request_id", uuid.New())))
ctx = logging.NewContext(ctx, logger)
if msg, ok := req.(fmt.Stringer); ok {
logger.With().Debug("new GRPC", log.Field(zap.Stringer("from", peer.Addr)), log.Field(zap.Stringer("message", msg)))
}
resp, err := handler(ctx, req)
if err != nil {
logger.With().Info("FAILURE", log.Err(err))
}
return resp, err
}
}

The returned error will be visible in logs printed there.

service/round.go Show resolved Hide resolved
@poszu poszu merged commit 301d2bb into develop Dec 16, 2022
@poszu poszu deleted the pm-173-remove-proof-broadcasting branch December 16, 2022 08:43
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 27, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 27, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 28, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 28, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 28, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
bors bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Dec 28, 2022
## Motivation
Part of spacemeshos/pm#173

Closes #3746 
Closes #3814 

## Changes
- removed broadcasting method from `GatewayService`
- removed p2p listeners for broadcasted poet proofs
- changed `NIPostBuilder` to query poets for proofs after the rounds end

## Test Plan
- added a system test in which nodes use different poets to verify if poet proofs are properly propagated between nodes

## TODO
- [ ] Bump poet to a released version in go.mod after spacemeshos/poet#187 is merged

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] ~This PR does not affect public APIs~ Proof broadcasting was removed
- [ ] ~This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)~ - It relies on a new Poet version
- [ ] ~This PR does not make changes to log messages (which monitoring infrastructure may rely on)~


Co-authored-by: moshababo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants