feat!: implement `FillSquare` method for the testnode #866

evan-forbes · 2022-10-13T00:36:42Z

This PR

adds a modified FillSquare method for the testnode that only submits a single large message
modifies the default configs for a testnode's mempool to accept large transactions
modifies the PostData method to accept a flag that determines if the process blocks

The issue with the old approach was that it was submitting too many transactions for a block with such short block times. The testnode was unlikely to get all of the transactions included in a single block, particularly when waiting to submit each transaction until the previous one was accepted into the mempool.

codecov-commenter · 2022-10-13T00:43:06Z

Codecov Report

Merging #866 (74cbca3) into main (be5406b) will increase coverage by 0.16%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##             main     #866      +/-   ##
==========================================
+ Coverage   25.59%   25.75%   +0.16%     
==========================================
  Files          75       75              
  Lines        9273     9287      +14     
==========================================
+ Hits         2373     2392      +19     
+ Misses       6677     6675       -2     
+ Partials      223      220       -3

Impacted Files	Coverage Δ
pkg/prove/proof.go	`83.56% <ø> (-0.12%)`	⬇️
testutil/testnode/node_interaction_api.go	`60.00% <60.00%> (-0.23%)`	⬇️
pkg/da/data_availability_header.go	`81.14% <100.00%> (+2.45%)`	⬆️
testutil/testnode/full_node.go	`75.29% <100.00%> (+0.29%)`	⬆️
x/payment/types/wirepayfordata.go	`68.69% <0.00%> (+2.33%)`	⬆️
pkg/shares/non_interactive_defaults.go	`100.00% <0.00%> (+5.17%)`	⬆️

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

rach-id

LGTM, only lefts comments to understand more

testutil/testnode/full_node_test.go

testutil/testnode/node_interaction_api.go

rach-id · 2022-10-13T00:59:02Z

testutil/testnode/node_interaction_api.go

+	}
+	maxShareCount := squareSize * squareSize
+	// we use a formula to guarantee that the tx is the exact size needed to force a specific square size.
+	msgSize := (maxShareCount - (2 * squareSize)) * appconsts.SparseShareContentSize


[Non Blocking][Question]
The (2 * squareSize) refers to the size that the transaction will take, aside from the message?

Assume squareSize=16 so maxShareCount=256:

msgSize := (maxShareCount - (2 * squareSize)) * appconsts.SparseShareContentSize msgSize := (256 - (2 * 16)) * appconsts.SparseShareContentSize msgSize := (256 - 32) * appconsts.SparseShareContentSize msgSize := (224) * appconsts.SparseShareContentSize

I don't think the transaction is guaranteed to take (2 * squareSize) but I think it is an approximation for the other shares in the square (transactions, intermediate state roots, evidence).

yeah, what Rootul said. In order to get the square size we want, we just have to make sure that we're using some number of share in between maxShareCount and minShareCount

maxShareCount := squareSize * squareSize minShareCount := maxShareCount / 2

Thanks a lot for your help 🙏

I think it is an approximation for the other shares in the square (transactions, intermediate state roots, evidence).

Another question, are ISRs and evidence currently added to blocks?

minShareCount := maxShareCount / 2

Why is that? is it to accommodate ISRs and evidence?

testutil/testnode/full_node_test.go

rootulp

🚀 you're a beast, great work!

testutil/testnode/full_node_test.go

rootulp · 2022-10-13T01:25:25Z

testutil/testnode/node_interaction_api.go

+
+// FillBlock creates and submits a single transaction that is large enough to
+// create a square of the desired size. broadcast mode indicates if the tx
+// should be submitted async, sync, or block. (see flags.BroadcastModeSync). If


[question] should this be flags.BroadcastMode per https://github.com/cosmos/cosmos-sdk/blob/7781cdb3d20bc7ebac017452897ce1e6ab3903ef/client/flags/flags.go#L114

Suggested change

// should be submitted async, sync, or block. (see flags.BroadcastModeSync). If

// should be submitted async, sync, or block (see flags.BroadcastMode). If

Note: block mode was removed in cosmos/cosmos-sdk#12659. Since celestia-app currently uses cosmos-sdk v0.46.0 we can still use block here but note to future selves that we'll need to remove it when we upgrade cosmos-sdk to a release with that change

yeah, blocking is just convenient atm and we will have to fix this in the future across the board.

rootulp · 2022-10-13T01:30:46Z

testutil/testnode/node_interaction_api.go

+	}
+	maxShareCount := squareSize * squareSize
+	// we use a formula to guarantee that the tx is the exact size needed to force a specific square size.
+	msgSize := (maxShareCount - (2 * squareSize)) * appconsts.SparseShareContentSize


Assume squareSize=16 so maxShareCount=256:

msgSize := (maxShareCount - (2 * squareSize)) * appconsts.SparseShareContentSize msgSize := (256 - (2 * 16)) * appconsts.SparseShareContentSize msgSize := (256 - 32) * appconsts.SparseShareContentSize msgSize := (224) * appconsts.SparseShareContentSize

I don't think the transaction is guaranteed to take (2 * squareSize) but I think it is an approximation for the other shares in the square (transactions, intermediate state roots, evidence).

rootulp · 2022-10-13T01:33:12Z

testutil/testnode/full_node_test.go

+func (s *IntegrationTestSuite) Test_FillBlock() {
+	require := s.Require()
+
+	for squareSize := 16; squareSize < appconsts.MaxSquareSize; squareSize *= 2 {


[question] out of curiosity, why start squareSize at 16? My guess is that the estimation logic for non message-shares may break for squareSizes < 16.

good catch, that was a remnant of the old implementation.

57744bf

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

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

…ledger-app into evan/implement-fillnode

evan-forbes · 2022-10-13T08:52:59Z

we should cut a release for v0.7.0 after merging this imo

This PR - adds a modified FillSquare method for the testnode that only submits a single large message - modifies the default configs for a testnode's mempool to accept large transactions - modifies the `PostData` method to accept a flag that determines if the process blocks The issue with the old approach was that it was submitting too many transactions for a block with such short block times. The testnode was unlikely to get all of the transactions included in a single block, particularly when waiting to submit each transaction until the previous one was accepted into the mempool. Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Rootul P <rootulp@gmail.com>

evan-forbes added 2 commits October 12, 2022 19:19

chore: change the default test node configs to accept single large txs

4cbb7f3

feat!: implement fillsquare api

3d648a9

evan-forbes added the testing items that are strictly related to adding or extending test coverage label Oct 13, 2022

evan-forbes requested review from rootulp and rach-id October 13, 2022 00:36

evan-forbes self-assigned this Oct 13, 2022

chore: linter

6a96fc3

evan-forbes mentioned this pull request Oct 13, 2022

chore: cherry-pick FillBlock #861

Closed

rach-id reviewed Oct 13, 2022

View reviewed changes

rootulp previously approved these changes Oct 13, 2022

View reviewed changes

rootulp mentioned this pull request Oct 13, 2022

chore!: upgrade to celestia-app v0.7.0 celestiaorg/celestia-node#1147

Merged

fix: more accurate error message

670e880

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

evan-forbes dismissed rootulp’s stale review via 670e880 October 13, 2022 08:07

evan-forbes and others added 4 commits October 13, 2022 03:11

fix: remove unused comment

a28147a

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

fix: test with smaller square sizes

1ed9ff6

Merge branch 'evan/implement-fillnode' of github.com:celestiaorg/lazy…

74cbca3

…ledger-app into evan/implement-fillnode

fix: use smaller test square size and document hackiness

57744bf

This was referenced Oct 13, 2022

testnode: Perform checks on squareSize argument in FillSquare method #867

Closed

Remove usage of BroadcastModeBlock #868

Open

evan-forbes enabled auto-merge (squash) October 13, 2022 08:49

evan-forbes mentioned this pull request Oct 13, 2022

typo: Fix func name QueryWithOutProof #869

Closed

rach-id approved these changes Oct 13, 2022

View reviewed changes

evan-forbes merged commit c9dce39 into main Oct 13, 2022

evan-forbes deleted the evan/implement-fillnode branch October 13, 2022 09:38

Bidon15 mentioned this pull request Oct 13, 2022

chore: upgrade test-infra to latest releases from node + app celestiaorg/test-infra#90

Merged

rootulp mentioned this pull request Oct 17, 2022

Cherry-pick testnode changes to main #860

Closed

adlerjohn mentioned this pull request Nov 1, 2022

EPIC: Add an official spec for the app #650

Closed

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: implement `FillSquare` method for the testnode #866

feat!: implement `FillSquare` method for the testnode #866

evan-forbes commented Oct 13, 2022

codecov-commenter commented Oct 13, 2022 •

edited

Loading

rach-id left a comment

rach-id Oct 13, 2022

rootulp Oct 13, 2022

evan-forbes Oct 13, 2022

rach-id Oct 13, 2022

rootulp left a comment

rootulp Oct 13, 2022

evan-forbes Oct 13, 2022

evan-forbes Oct 13, 2022

rootulp Oct 13, 2022

rootulp Oct 13, 2022

evan-forbes Oct 13, 2022

evan-forbes Oct 13, 2022

evan-forbes commented Oct 13, 2022

	// should be submitted async, sync, or block. (see flags.BroadcastModeSync). If
	// should be submitted async, sync, or block (see flags.BroadcastMode). If

feat!: implement FillSquare method for the testnode #866

feat!: implement FillSquare method for the testnode #866

Conversation

evan-forbes commented Oct 13, 2022

codecov-commenter commented Oct 13, 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

rootulp 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

evan-forbes commented Oct 13, 2022

feat!: implement `FillSquare` method for the testnode #866

feat!: implement `FillSquare` method for the testnode #866

codecov-commenter commented Oct 13, 2022 •

edited

Loading