feat!: Refactor PrepareProposal to produce blocks using the non-interactive defaults
#692
Conversation
evan-forbes
added
C: Celestia app
consensus breaking
evan-forbes
requested review from
adlerjohn,
rootulp,
liamsi,
mojtaba-esk,
rahulghangas and
rach-id
Codecov Report
@@ Coverage Diff @@
## main #692 +/- ##
==========================================
- Coverage 46.38% 42.16% -4.23%
==========================================
Files 33 42 +9
Lines 3251 4383 +1132
==========================================
+ Hits 1508 1848 +340
- Misses 1625 2387 +762
- Partials 118 148 +30
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| // TODO: redo this tests, which is more difficult to do now that it requires the | ||
| // data to be processed by PrepareProposal func | ||
| // TestProcessMessagesWithReservedNamespaces(t *testing.T) { |
There was a problem hiding this comment.
this test was commented out as its still something we want, although I think it can be done in a different PR. The modified test above kind of tests this, but its difficult to perform the same test since we've added requirements to how we arrange the block. We basically have to call PrepareProposal in order for ProcessProposal to accept the block, which itself checks for these invalid txs.
| func TestMessageInclusionCheck(t *testing.T) { | ||
| signer := testutil.GenerateKeyringSigner(t, testAccName) |
There was a problem hiding this comment.
refactored this test to also test for the deleted test
testutil/payment/testutil.go
Outdated
| // GenerateManyRawWirePFD creates many raw WirePayForData transactions. Using | ||
| // negative numbers for count and size will randomize those values. count is | ||
| // capped at 5000 and size is capped at 3MB. Going over these caps will result | ||
| // in randomized values. | ||
| func GenerateManyRawWirePFD(t *testing.T, txConfig client.TxConfig, signer *types.KeyringSigner, count, size int) [][]byte { |
There was a problem hiding this comment.
we have a lot of duplicate test code that we need to sort out
Co-authored-by: Rootul P <rootulp@gmail.com>
we can try and do this. It might mean duplicate or unused code in some instances, or that we have to spend more time debugging getting that chunk of code working with the old implementation, but those things might be unavoidable in order to get a proper review. We also have roughly 2.5 weeks until this needs to be finished and merged now that the audit has been postponed a bit, so we should have the necessary time. will likely close this PR after opening new smaller ones. |
|
Blocked on #691 |
This preserves the ability to keep tests in their respective packages.
evan-forbes
changed the title
PrepareProposal to produce blocks using the non-interactive defaultsPrepareProposal to produce blocks using the non-interactive defaults
|
I haven't made many changes other than to make compatible with main. Reopening after attempting to break into a few PRs. While possible to break into smaller chunks, its impossible to add tests to those chunks until all portions are functional. |
| removedContiguousShares := 0 | ||
| contigBytesCursor := 0 |
There was a problem hiding this comment.
[follow-up PR] we may replace the contiguous terminology with compact
| @@ -160,5 +160,5 @@ require ( | |||
| replace ( | |||
| github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.2.0-sdk-v0.46.0 | |||
| github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 | |||
| github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20 | |||
| github.com/tendermint/tendermint v0.34.20 => github.com/celestiaorg/celestia-core v1.5.0-tm-v0.34.20 | |||
There was a problem hiding this comment.
[question][non-blocking] out of curiosity, why is the exact tendermint version specified?
There was a problem hiding this comment.
whoops, good catch, this actually wasn't on purpose. While it technically shouldn't matter we might want to switch it back. I think the only time it did matter was when used to change the imports from tendermint/tendermint to celestiaorg/celestia-core
There was a problem hiding this comment.
merging w/o as this is optional, and I don't want to look at this PR anymore lol
There was a problem hiding this comment.
Agreed. 
and we can always remove later
…eractive defaults (celestiaorg#692) Co-authored-by: Rootul Patel <rootulp@gmail.com>
This PR refactors
PrepareProposalto produce blocks according to the non-interactive defaults. It could probably be split up into a few PRs, and we might have to do that to make it easier to review.a big part of #382
(we can optionally spin out some changes into a different PR, but this PR was blocked on the below issues, so I included the relevant changes here (pls see comments for details))
closes #693
closes #588