feat: Implement an in-process test core node with celestia-app and custom state #824

evan-forbes · 2022-09-30T23:47:57Z

This PR is based of the network tests in the cosmos-sdk. Those tests are focused on running multiple nodes, while this test only runs a single node validator. This node also has the full access to change the consensus parameters and the full tendermint config. This is needed to to have block times shorter than 1 second. There is also discussion to modify the sdk code to only be a mock, where we need to run a full in process node to include any changes to tendermint and to not have to apply those same changes to the new upcoming mock node in the sdk.

While this does have extra features and a built in API, it does have a lot of duplicate features to our own wrapper around the linked above network tests. Eventually we should replace the usage of that code in the integrations tests with this code.

closes #770

…h celestia-app

…is txs

codecov-commenter · 2022-09-30T23:55:28Z

Codecov Report

Merging #824 (66f7d9d) into main (2808660) will decrease coverage by 16.80%.
The diff coverage is 67.54%.

@@             Coverage Diff             @@
##             main     #824       +/-   ##
===========================================
- Coverage   42.00%   25.20%   -16.81%     
===========================================
  Files          42       75       +33     
  Lines        4378     9225     +4847     
===========================================
+ Hits         1839     2325      +486     
- Misses       2390     6681     +4291     
- Partials      149      219       +70

Impacted Files	Coverage Δ
testutil/testnode/node_interaction_api.go	`60.22% <60.22%> (ø)`
testutil/testnode/node_init.go	`66.48% <66.48%> (ø)`
testutil/testnode/full_node.go	`72.36% <72.36%> (ø)`
testutil/testnode/rpc_client.go	`81.81% <81.81%> (ø)`
pkg/shares/shares.go	`22.85% <0.00%> (-77.15%)`	⬇️
pkg/shares/parse_sparse_shares.go	`77.33% <0.00%> (-2.67%)`	⬇️
x/payment/types/payfordata.go	`76.33% <0.00%> (-0.36%)`	⬇️
app/app.go	`6.49% <0.00%> (-0.07%)`	⬇️
pkg/inclusion/paths.go	`100.00% <0.00%> (ø)`
x/payment/types/tx.pb.go	`18.04% <0.00%> (ø)`
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rach-id

utAck. Awesome stuff 🚀 Would be really helpful for QGB tests 🔥

rach-id · 2022-10-01T00:13:23Z

testutil/testnode/node_init.go

+	}
+
+	cparams := types.DefaultConsensusParams()
+	cparams.Block.TimeIotaMs = 1


[Non Blocking][Question]
Doesn't it make sense to take the consensus params as a parameter for more flexibility?
Especially the block time, I guess it makes sense to give the user the abilitiy to change it.
Then, we can for example check if the provided consensus params are null, we initialize using the default ones.

good idea 903bc0b

rach-id · 2022-10-01T00:16:29Z

testutil/testnode/node_interaction_api.go

+	var latestHeight int64
+	for {
+		select {
+		case <-timeout.C:


[Non Blocking][Suggestion]
Doesn't it make sense also to check if the root context was cancelled or done?

👍 96cec2a

rach-id · 2022-10-01T00:17:02Z

testutil/testnode/node_interaction_api.go

+	ticker := time.NewTicker(time.Second)
+	defer ticker.Stop()
+
+	timeout := time.NewTimer(t)


[Non Blocking][Suggestion]
Doesn't it make sense to use the context with timeout? to also propagate timeouts if something happens?

good point, might as well 96cec2a

rach-id · 2022-10-01T00:18:33Z

testutil/testnode/node_interaction_api.go

+
+	// create a random msg per row
+	pfd, err := payment.BuildPayForData(
+		context.TODO(),


[Non Blocking][Question]
Why use the TODO context when we have a context passed func (c *Context) PostData...?

nice catch! wrote this when we didn't yet have contexts 95529af

rach-id · 2022-10-01T00:21:40Z

testutil/utils.go

+func RandomValidNamespace() namespace.ID {
+	for {
+		s := tmrand.Bytes(8)
+		if bytes.Compare(s, appconsts.MaxReservedNamespace) > 0 {


[Non Blocking][Question]
This one is used to create a PFD. Shouldn't we also check if the namespace is not the ns of evidence or?

MaxReservedNamespace is greater than the namespace used for transactions, intermediate state roots, or evidence so this conditional will evaluate to false if s matches any of those namespaces.

In other words, the returned s should only be valid message namespaces, TailPaddingNamespaceID or ParitySharesNamespaceID

https://github.com/celestiaorg/celestia-app/blob/main/pkg/appconsts/appconsts.go#L90-L96

Sorry yes, I meant tail padding or parity. Is that alright?

nice catch! 6f50ff6

rach-id · 2022-10-01T00:22:09Z

testutil/utils.go

+		return nil, err
+	}
+
+	return node.Tx(context.Background(), hash, false)


[Question][Non Blocking]
Why using the background context when we have the clientCtx?

the clientCtx doesn't actually have a context.Context in it, and I didn't want to force the testnode package as a dependency. This is good point tho

rootulp · 2022-10-01T02:04:29Z

testutil/testnode/full_node.go

+// note: the passed application config is currently unused atm, but we plan to
+// add support.


[question] which parameter corresponds to the application config?

If present, proposal to explicitly include the parameter name in the note (because I don't think it's tmCfg).
If no longer present, proposal to remove this note.

good catch 14aca81

testutil/testnode/rpc_client.go

rootulp · 2022-10-01T02:18:37Z

testutil/utils.go

+func RandomValidNamespace() namespace.ID {
+	for {
+		s := tmrand.Bytes(8)
+		if bytes.Compare(s, appconsts.MaxReservedNamespace) > 0 {


MaxReservedNamespace is greater than the namespace used for transactions, intermediate state roots, or evidence so this conditional will evaluate to false if s matches any of those namespaces.

In other words, the returned s should only be valid message namespaces, TailPaddingNamespaceID or ParitySharesNamespaceID

https://github.com/celestiaorg/celestia-app/blob/main/pkg/appconsts/appconsts.go#L90-L96

Co-authored-by: Rootul P <rootulp@gmail.com>

…t data

…very fast block times

evan-forbes · 2022-10-03T20:04:03Z

had to skip the network test while the race detector is on, exactly like what we do for the other larger network test. a376539

rootulp · 2022-10-03T21:20:31Z

testutil/utils.go

+		ns := tmrand.Bytes(8)
+		isReservedNS := bytes.Compare(ns, appconsts.MaxReservedNamespace) <= 0
+		isParityNS := bytes.Equal(ns, appconsts.ParitySharesNamespaceID)
+		isTailPaddingNS := bytes.Equal(ns, appconsts.TailPaddingNamespaceID)
+		if isReservedNS || isParityNS || isTailPaddingNS {
+			continue
 		}
+		return ns


[question] Nice! There are multiple implementations of this function that may benefit from similar parity / tail padding namespace checks:

https://github.com/rootulp/celestia-app/blob/2f26c385831048808d99f4626aa3569df18c187c/app/test_util.go#L178-L185

https://github.com/rootulp/celestia-app/blob/2f26c385831048808d99f4626aa3569df18c187c/testutil/payment/testutil.go#L134-L141

https://github.com/rootulp/celestia-app/blob/2f26c385831048808d99f4626aa3569df18c187c/app/test/block_size_test.go#L299-L306

https://github.com/rootulp/celestia-app/blob/2f26c385831048808d99f4626aa3569df18c187c/pkg/prove/proof_test.go#L249-L257

It's not clear to me if these should be refactored into one function definition. Were they intentionally duplicated to avoid cyclic imports / importing across package boundaries?

…stom state (celestiaorg#824) This PR is based of the [network](https://github.com/cosmos/cosmos-sdk/tree/main/testutil/network) tests in the cosmos-sdk. Those tests are focused on running multiple nodes, while this test only runs a single node validator. This node also has the full access to change the consensus parameters and the full tendermint config. This is needed to to have block times shorter than 1 second. There is also discussion to modify the sdk code to only be a mock, where we need to run a full in process node to include any changes to tendermint and to not have to apply those same changes to the new upcoming mock node in the sdk. While this does have extra features and a built in API, it does have a lot of duplicate features to our own wrapper around the linked above network tests. Eventually we should replace the usage of that code in the integrations tests with this code. closes celestiaorg#770 Co-authored-by: Rootul P <rootulp@gmail.com>

evan-forbes added 7 commits September 22, 2022 16:27

feat: implemented ability to create a tendermint node initialized wit…

5db77f3

…h celestia-app

fix: flesh out node init

ce55526

feat: finish first draft of testnode API

58489d9

fix: remove commented out code

ba77967

fix: don't override the consensus paremters when collecting the genes…

9817864

…is txs

feat: add to api

2bcdadb

refactor: remove Fillblock and replace with PostData

e340e09

evan-forbes added C: Celestia app testing items that are strictly related to adding or extending test coverage labels Sep 30, 2022

evan-forbes self-assigned this Sep 30, 2022

chore: linter

66f7d9d

evan-forbes requested review from rootulp and rach-id September 30, 2022 23:53

rach-id reviewed Oct 1, 2022

View reviewed changes

rootulp previously approved these changes Oct 1, 2022

View reviewed changes

evan-forbes mentioned this pull request Oct 3, 2022

TestIntegrationTestSuite/TestMaxBlockSize flake #574

Closed

4 tasks

Apply suggestions from code review

8588dff

Co-authored-by: Rootul P <rootulp@gmail.com>

evan-forbes dismissed rootulp’s stale review via 8588dff October 3, 2022 17:39

evan-forbes added 6 commits October 3, 2022 12:40

fix: remove old docs

14aca81

fix: reduce flakiness with params query test

903bc0b

refactor: use context.WithTimeout instead of an independent timeout

96cec2a

fix: pass existing context

95529af

fix: check for parity and tail padding namespaces when generating tes…

6f50ff6

…t data

fix: add race detector skip because it breaks for network tests with …

a376539

…very fast block times

evan-forbes changed the title ~~feat!: Implement an in-process test core node with celestia-app and custom state~~ feat: Implement an in-process test core node with celestia-app and custom state Oct 3, 2022

evan-forbes requested review from rootulp and rach-id October 3, 2022 20:42

evan-forbes enabled auto-merge (squash) October 3, 2022 20:42

rootulp approved these changes Oct 3, 2022

View reviewed changes

evan-forbes merged commit 07b5bb4 into main Oct 3, 2022

evan-forbes deleted the evan/test-node branch October 3, 2022 21:20

evan-forbes mentioned this pull request Oct 4, 2022

refactor(swamp): use celestia-app instead of the kvstore celestiaorg/celestia-node#1160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement an in-process test core node with celestia-app and custom state #824

feat: Implement an in-process test core node with celestia-app and custom state #824

evan-forbes commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 •

edited

Loading

rach-id left a comment

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rach-id Oct 1, 2022

rootulp Oct 1, 2022

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rach-id Oct 1, 2022

evan-forbes Oct 3, 2022

rootulp Oct 1, 2022

evan-forbes Oct 3, 2022

rootulp Oct 1, 2022

evan-forbes commented Oct 3, 2022

rootulp Oct 3, 2022

		// note: the passed application config is currently unused atm, but we plan to
		// add support.

feat: Implement an in-process test core node with celestia-app and custom state #824

feat: Implement an in-process test core node with celestia-app and custom state #824

Conversation

evan-forbes commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 • edited Loading

Codecov Report

rach-id left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Oct 3, 2022

Choose a reason for hiding this comment

codecov-commenter commented Sep 30, 2022 •

edited

Loading