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

Validate and restore listeners on startup, and allow deletion if not found in connector #1234

Merged
merged 5 commits into from
Mar 27, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Mar 22, 2023

Had a situation where we wanted to restore fresh listeners for custom contracts on a FireFly core service, and had assumed FireFly core would re-create the listeners. So deleted them on the EVMConnect runtime directly and restarted FF.

Found related issues, that meant we had to go into the FF DB and remove the contract listeners directly (and have the app recreate them):

  1. FireFly did not recreate the listener. This seems a little inconsistent with both the BatchPin listener, and the listeners of the token connectors...
    • Proposing in this PR that we automatically recreate the listeners where possible (only Ethereum in this PR, due to existing limitations around the Fabric connector integration where it does not have existing query support to check existence)
  2. The API could not be used to delete the listener, because when EVMConnect returned a 404 FireFly failed the DELETE API.
    • This seemed like a straight bug to me. Due to the return of status being interface{} and the complexity of a nil check there, we're adding an extra bool to whether it's found or not (Fabric always returns true)
  3. Found in testing this change, that the check on existence of a contract listener with the same parameters was not working, passing in a []byte for the location rather than string

…leted

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst marked this pull request as ready for review March 22, 2023 23:11
@peterbroadhurst
Copy link
Contributor Author

Adding fix for this build failure:

--- FAIL: TestStartLegacyAdminConfig (0.01s)
    server_test.go:116: 
        	Error Trace:	/home/runner/work/firefly/firefly/internal/apiserver/server_test.go:116
        	Error:      	Received unexpected error:
        	            	listen tcp 127.0.0.1:6000: bind: address already in use
        	            	FF00151: Unable to start listener on 127.0.0.1:6000: %!s(MISSING)
        	            	github.com/hyperledger/firefly-common/pkg/i18n.WrapError
        	            		/home/runner/go/pkg/mod/github.com/hyperledger/firefly-common@v1.2.3/pkg/i18n/errors.go:42
        	            	github.com/hyperledger/firefly-common/pkg/httpserver.(*httpServer).createListener
        	            		/home/runner/go/pkg/mod/github.com/hyperledger/firefly-common@v1.2.3/pkg/httpserver/httpserver.go:97
        	            	github.com/hyperledger/firefly-common/pkg/httpserver.NewHTTPServer
        	            		/home/runner/go/pkg/mod/github.com/hyperledger/firefly-common@v1.2.3/pkg/httpserver/httpserver.go:82
        	            	github.com/hyperledger/firefly/internal/apiserver.(*apiServer).Serve
        	            		/home/runner/work/firefly/firefly/internal/apiserver/server.go:110
        	            	github.com/hyperledger/firefly/internal/apiserver.TestStartLegacyAdminConfig
        	            		/home/runner/work/firefly/firefly/internal/apiserver/server_test.go:115
        	            	testing.tRunner
        	            		/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439
        	            	runtime.goexit
        	            		/opt/hostedtoolcache/go/1.18.10/x64/src/runtime/asm_amd64.s:1571
        	Test:       	TestStartLegacyAdminConfig

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Comment on lines +112 to +114
// cause recreation of all the listeners (noting that listeners that were specified to start
// from latest, will start from the new latest rather than replaying from the block they
// started from before they were deleted).
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if, rather than storing "latest" we should resolve what the latest block number is at the time, then store that? This is out of scope for this PR, but may be worth thinking about for better determinism.

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

One minor thing which I think will result in a misleading log message, but other than that, looks good!

Comment on lines 223 to 224
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to increment verifyCount right here. Currently it doesn't look like it's ever updated.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst merged commit 217e101 into hyperledger:main Mar 27, 2023
@peterbroadhurst peterbroadhurst deleted the validate-listeners branch March 27, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants