celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

chore(deps): upgrade to celestia-app v2.0.0-rc2

Open rootulp opened this issue 1 year ago • 4 comments

Update celestia-node to use celestia-app v2.0.0-rc2. A few changes that were needed:

  • celestia-app v1.x had a shares package. celestia-app v2.x uses the shares package from go-square.
  • celestia-app v1.x had a blob.types package with CreateCommitment. celestia-app v2.x uses CreateCommitment from the go-square inclusion package.
  • celestia-app v1.x had a lot of functionality included in the signer. celestia-app v2.x split a txClient from the signer. See: https://github.com/celestiaorg/celestia-app/pull/3433
  • I had to update extended header verification to allow header.Version.App = 2. Added unit tests.
  • I had to update core_access.go a lot. Mostly inspired by https://github.com/celestiaorg/celestia-node/pull/3451

Testing

I ran a local celestia-app v2.0.0-rc1 node and configured it to upgrade at block height 5. celestia-node (built from this PR) continued to work:

2024-06-07T16:40:35.129-0400	INFO	header/store	store/store.go:367	new head	{"height": 4, "hash": "FA6E8DD2D20088AE81724D12064CEC489CFD93C0069FD92B4623F7C9DB26DC9D"}
2024-06-07T16:40:46.157-0400	INFO	header/store	store/store.go:367	new head	{"height": 5, "hash": "94422C30BEAF1C76C30FF73B5C00D3777275696DC8128E62B77719236BE4BDAA"}
2024-06-07T16:40:57.180-0400	INFO	header/store	store/store.go:367	new head	{"height": 6, "hash": "5084410C87A48702DDD0066E9F6A9D25A7E70BB3CBC6DD9B21694C682E398A77"}
2024-06-07T16:41:08.209-0400	INFO	header/store	store/store.go:367	new head	{"height": 7, "hash": "67385DF7026E51ED0C0AAF7D8434356A7B3B132A23B78751C6B3C3BF18362B6A"}

I also ran a local celestia-app v1.9.0 node and celestia-node (build from this PR) continued to work.

rootulp avatar May 31 '24 18:05 rootulp

It looks like there was a BroadcastTx on v1.x that no longer exists on celestia-app v2.x so I'll copy it here.

rootulp avatar Jun 03 '24 15:06 rootulp

Failing CI 😢

rootulp avatar Jul 02 '24 19:07 rootulp

Integration tests are failing with

        }': signature verification failed; please verify account number (0) and chain-id (private): unauthorized

I think there may be a bug in celestia-app v2.x testnode implementation where it fails to propagate a custom chain-id to the thing responsible for signing the create validator transactions.

rootulp avatar Jul 04 '24 04:07 rootulp

This PR is now blocked on https://github.com/celestiaorg/celestia-app/issues/3662

rootulp avatar Jul 04 '24 04:07 rootulp

Note to self: look at https://github.com/celestiaorg/celestia-node/pull/3349 to resolve merge conflicts

rootulp avatar Jul 08 '24 14:07 rootulp

TestTransfer fails because the core accessor returns a min gas price of "" which gets converted to 0. .002 is expected.

rootulp avatar Jul 09 '24 22:07 rootulp

  • I confirmed that the appConfig is getting overriden to supply a custom min gas price. This also repros if we use the default min gas price instead of overriding it.
  • It looks like the sdkCtx doesn't contain a populated minGasPrice here.
  • I can repro the test failing in celestia-app on v2.0.0-rc2.
  • I can't repro on celestia-app v1.12.0.

rootulp avatar Jul 10 '24 14:07 rootulp

Integration tests fail:

--- FAIL: TestBlobRPC (10.31s)
    api_test.go:121:
        	Error Trace:	/Users/rootulp/git/rootulp/celestiaorg/celestia-node/nodebuilder/tests/api_test.go:121
        	Error:      	Received unexpected error:
        	            	fatal error calling 'blob.Submit': panic in rpc method 'blob.Submit': runtime error: invalid memory address or nil pointer dereference
        	Test:       	TestBlobRPC
    network.go:46: tearing down testnode

I think because the tx_client doesn't get initialized with a mtx here.

rootulp avatar Jul 16 '24 18:07 rootulp

Codecov Report

Attention: Patch coverage is 97.43590% with 3 lines in your changes missing coverage. Please review.

Project coverage is 45.73%. Comparing base (2469e7a) to head (e9eee2a). Report is 155 commits behind head on main.

Files Patch % Lines
blob/service.go 66.66% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3453      +/-   ##
==========================================
+ Coverage   44.83%   45.73%   +0.89%     
==========================================
  Files         265      281      +16     
  Lines       14620    16056    +1436     
==========================================
+ Hits         6555     7343     +788     
- Misses       7313     7876     +563     
- Partials      752      837      +85     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 17 '24 19:07 codecov-commenter

Integration tests fraud failed in CI but it passes locally:

$ make test-integration TAGS=fraud
--> Running integrations tests  -tags=fraud
ok  	github.com/celestiaorg/celestia-node/nodebuilder/tests	64.710s

rootulp avatar Jul 17 '24 19:07 rootulp

Finally passing CI 😅

rootulp avatar Jul 17 '24 20:07 rootulp

@renaynay TestListener fails here because header validation fails:

2024-08-01T19:58:59.722Z	ERROR	header/p2p	p2p/subscriber.go:122	invalid header	{"from": "<peer.ID Qm*JPpk76>", "err": "app version mismatch, expected: 1, got 2"}

so I plan on reverting 79d8458 and then you can still rebase your PR on top of this one (or merge it into this one, whichever you prefer)

rootulp avatar Aug 01 '24 20:08 rootulp