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

Do not accept new tx via API if node is not in sync #2114

Closed
wants to merge 7 commits into from

Conversation

hoenirvili
Copy link
Contributor

@hoenirvili hoenirvili commented Aug 11, 2020

Motivation

When we submit a transaction we should verify if the node is in sync first.

Closes #2103

Changes

I've added two checks one that checks if the transaction byte payload is even filled and one for testing the equality between the number of layers processed and the last layer in state.

Test Plan

Looks like the TransactionService by default is not turn on and I did first turned on the transaction service in code (config) and then started the node(that is not in sync) and executed a grpcurl comand with the xdr serialized payload.

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed

api/grpcserver/transaction_service.go Show resolved Hide resolved
api/grpcserver/transaction_service.go Outdated Show resolved Hide resolved
@lrettig
Copy link
Member

lrettig commented Aug 11, 2020

Thanks for the PR. See comments I added. Could we also add a unit test for this? See the unit tests at

func TestTransactionService(t *testing.T) {
.

Basically you'd want to mock the Syncer (or use the existing mock), make it report that the node is NOT synced, then submit a tx and make sure it produces an error. Then make the syncer report that the node is synced, submit a tx again and make sure it goes through without an error.

@lrettig lrettig assigned lrettig and unassigned lrettig Aug 11, 2020
@lrettig lrettig added the API label Aug 11, 2020
@lrettig
Copy link
Member

lrettig commented Aug 11, 2020

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 11, 2020

If it's not too much trouble, could you post the stack trace here? Thanks!

Sure

grpcurl -d '{}' -plaintext localhost:9092 spacemesh.v1.TransactionService.SubmitTransaction
2020-08-12T00:24:38.527+0300    INFO    00000.defaultLogger     GRPC TransactionService.SubmitTransaction
2020-08-12T00:24:38.527+0300    ERROR   00000.defaultLogger     failed to deserialize tx, error: xdr:DecodeUhyper: EOF while decoding 8 bytes - read: '[]'
2020-08-12T00:24:38.527+0300    INFO    00000.defaultLogger     not reporting error as buffer is full: {failed to deserialize tx, error: xdr:DecodeUhyper: EOF while decoding 8 bytes - read: '[]' goroutine 2324 [running]:
runtime/debug.Stack(0x0, 0xc0138b86c0, 0xc0029c94b0)
        /usr/lib/go/src/runtime/debug/stack.go:24 +0x9d
github.com/spacemeshos/go-spacemesh/cmd/node.(*SpacemeshApp).setupLogging.func1(0x2, 0xbfc4e1059f6dbab5, 0x2d870d8e68, 0x19e6260, 0xff28c6, 0x13, 0xc00029e2a0, 0x5a, 0x0, 0x0, ...)
        /home/frelsari/Work/go-spacemesh/cmd/node/node.go:288 +0x42
go.uber.org/zap/zapcore.(*hooked).Write(0xc000457380, 0x2, 0xbfc4e1059f6dbab5, 0x2d870d8e68, 0x19e6260, 0xff28c6, 0x13, 0xc00029e2a0, 0x5a, 0x0, ...)
        /home/frelsari/go/pkg/mod/go.uber.org/zap@v1.10.0/zapcore/hook.go:65 +0x9b
go.uber.org/zap/zapcore.(*CheckedEntry).Write(0xc012ce42c0, 0x0, 0x0, 0x0)
        /home/frelsari/go/pkg/mod/go.uber.org/zap@v1.10.0/zapcore/entry.go:215 +0x117
go.uber.org/zap.(*SugaredLogger).log(0xc000120480, 0x2, 0x1007012, 0x23, 0xc0029c98e0, 0x1, 0x1, 0x0, 0x0, 0x0)
        /home/frelsari/go/pkg/mod/go.uber.org/zap@v1.10.0/sugar.go:234 +0x100
go.uber.org/zap.(*SugaredLogger).Errorf(...)
        /home/frelsari/go/pkg/mod/go.uber.org/zap@v1.10.0/sugar.go:148
github.com/spacemeshos/go-spacemesh/log.Log.Error(...)
        /home/frelsari/Work/go-spacemesh/log/zap.go:36
github.com/spacemeshos/go-spacemesh/log.Error(...)
        /home/frelsari/Work/go-spacemesh/log/log.go:150
github.com/spacemeshos/go-spacemesh/api/grpcserver.TransactionService.SubmitTransaction(0x7eff34c11398, 0xc0001da000, 0x11b1720, 0xc00273cd80, 0xc000025a40, 0x1182060, 0xc00028bb80, 0x119dca0, 0xc0138b8360, 0xc00007ec80, ...)
        /home/frelsari/Work/go-spacemesh/api/grpcserver/transaction_service.go:66 +0x139
github.com/spacemeshos/api/release/go/spacemesh/v1._TransactionService_SubmitTransaction_Handler(0xf6dd80, 0xc0001d2e80, 0x119dca0, 0xc0138b8360, 0xc01546c840, 0x0, 0x119dca0, 0xc0138b8360, 0x0, 0x0)
        /home/frelsari/go/pkg/mod/github.com/spacemeshos/api/release/go@v0.0.0-20200720193236-49b94a501b3e/spacemesh/v1/tx.pb.go:235 +0x217
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0001996c0, 0x11aad00, 0xc01868a600, 0xc00a75b300, 0xc010ba0660, 0x19b8380, 0x0, 0x0, 0x0)
        /home/frelsari/go/pkg/mod/google.golang.org/grpc@v1.29.1/server.go:1082 +0x50a
google.golang.org/grpc.(*Server).handleStream(0xc0001996c0, 0x11aad00, 0xc01868a600, 0xc00a75b300, 0x0)
        /home/frelsari/go/pkg/mod/google.golang.org/grpc@v1.29.1/server.go:1405 +0xccd
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc004ca8020, 0xc0001996c0, 0x11aad00, 0xc01868a600, 0xc00a75b300)
        /home/frelsari/go/pkg/mod/google.golang.org/grpc@v1.29.1/server.go:746 +0xa1
created by google.golang.org/grpc.(*Server).serveStreams.func1
        /home/frelsari/go/pkg/mod/google.golang.org/grpc@v1.29.1/server.go:744 +0xa1
 2}

 a transaction

Signed-off-by: hoenirvili <hoenirvili@gmail.com>
@hoenirvili hoenirvili force-pushed the node-sync-2103 branch 3 times, most recently from 762e217 to 626bd29 Compare Aug 12, 2020
Signed-off-by: hoenirvili <hoenirvili@gmail.com>
@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 13, 2020

@lrettig Can you check again the patch?

bors bot added a commit that referenced this pull request Aug 13, 2020
@bors
Copy link

bors bot commented Aug 13, 2020

try

Build failed:

@hoenirvili hoenirvili requested a review from lrettig Aug 13, 2020
Signed-off-by: hoenirvili <hoenirvili@gmail.com>
@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 18, 2020

Can you look one more time at the patch?

cmd/node/node.go Show resolved Hide resolved

// Syncer defines ways of interacting with the node syncer
// business logic
type Syncer interface {
Copy link
Member

@lrettig lrettig Sep 2, 2020

Choose a reason for hiding this comment

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

Please use the existing interface:

IsSynced() bool

Copy link
Contributor Author

@hoenirvili hoenirvili Sep 3, 2020

Choose a reason for hiding this comment

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

Why use this interface and implement Start as no-op? It doesn't make sense. I know it's good to reutilize the code but this basically states, "hey I do want Syncer and that Syncer can decide if it's synced or not and I don't require anything else".

Copy link
Member

@lrettig lrettig Sep 3, 2020

Choose a reason for hiding this comment

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

I'm not sure what you mean. You don't need to implement Start, or IsSynced. The app.syncer object that you pass in at https://github.com/spacemeshos/go-spacemesh/pull/2114/files#diff-a41cea38155fea30556048c9d149900fR742 already implements both of these.

Copy link
Contributor Author

@hoenirvili hoenirvili Sep 3, 2020

Choose a reason for hiding this comment

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

I'm talking about "interface-segregation principle". (no client should be forced to depend on methods it does not use). Given this argument, if you see the TransactionService shouldn't make clients implement the whole Syncer interface if the TransactionService dosen't require it.

api/grpcserver/grpcserver_test.go Outdated Show resolved Hide resolved
api/grpcserver/grpcserver_test.go Show resolved Hide resolved
api/grpcserver/transaction_service.go Outdated Show resolved Hide resolved
Signed-off-by: hoenirvili <hoenirvili@gmail.com>
@hoenirvili hoenirvili requested a review from lrettig Sep 3, 2020
@lrettig lrettig changed the title Check if we are in sync first Do not accept new tx via API if node is not in sync Sep 3, 2020
Copy link
Member

@lrettig lrettig left a comment

We're almost there! I made a few small tweaks. Let's clean up the last lingering comments and get this merged! 🚀


// Syncer defines ways of interacting with the node syncer
// business logic
type Syncer interface {
Copy link
Member

@lrettig lrettig Sep 3, 2020

Choose a reason for hiding this comment

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

I'm not sure what you mean. You don't need to implement Start, or IsSynced. The app.syncer object that you pass in at https://github.com/spacemeshos/go-spacemesh/pull/2114/files#diff-a41cea38155fea30556048c9d149900fR742 already implements both of these.

@bors bors bot closed this Sep 10, 2020
@lrettig
Copy link
Member

lrettig commented Sep 10, 2020

It looks like this was closed automatically since the base branch was merged into develop. I can't reopen this. We'll need to transfer this over to a new PR against develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants