Sync PrepareProposal with Spec. Main part#9158
Sync PrepareProposal with Spec. Main part#9158sergio-mena merged 16 commits intofeature/abci++pppfrom
Conversation
…rsion of the spec (#8094) This change implements the logic for the PrepareProposal ABCI++ method call. The main logic for creating and issuing the PrepareProposal request lives in execution.go and is tested in a set of new tests in execution_test.go. This change also updates the mempool mock to use a mockery generated version and removes much of the plumbing for the no longer used ABCIResponses.
|
Addresses #9118 |
| } else if block == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Good point. I checked the new logic of createProposalBlock and it seems it's not possible (either you get the block or an error).
I'll change the return by a panic.
There was a problem hiding this comment.
BTW, this is something I didn't catch as a reviewer last time (in #8094) 😉
| if err := blockExec.mempool.CheckTx(atx, nil, mempool.TxInfo{}); err != nil { | ||
| blockExec.logger.Error("error adding tx to the mempool", "error", err, "tx hash", atx.Hash()) | ||
| } |
There was a problem hiding this comment.
I didn't know we did that. But why do we need to run CheckTx again?
There was a problem hiding this comment.
These are transactions marked as Added, I believe the semantics at the time were that Tx marked added are placed into the mempool. Was then removed when we thought better of this idea in #8176
There was a problem hiding this comment.
Yeah Sergio followed up with me async. Thanks for the context
There was a problem hiding this comment.
Yeah, as soon as I get this PR merged I'll post the last PR, which contains #8176, which was fixing this on v0.36.x
| // all holds the complete list of all transactions from the original list of | ||
| // TxRecords. | ||
| all Txs |
There was a problem hiding this comment.
Is this necessary? Doesn't look like we use it
There was a problem hiding this comment.
It's used in the validation logic to check for duplicates.
* ----start---- * [PARTIAL cherry-pick] ABCI Vote Extension 2 (#6885) * Cherry-picked #6567: state/types: refactor makeBlock, makeBlocks and makeTxs (#6567) * [Cherrypicked] types: remove panic from block methods (#7501) * [cherrypicked] abci++: synchronize PrepareProposal with the newest version of the spec (#8094) This change implements the logic for the PrepareProposal ABCI++ method call. The main logic for creating and issuing the PrepareProposal request lives in execution.go and is tested in a set of new tests in execution_test.go. This change also updates the mempool mock to use a mockery generated version and removes much of the plumbing for the no longer used ABCIResponses. * make proto-gen * Backported EvidenceList's method ToABCI from #7961 * make build * Fix mockery for Mempool * mockery * Backported abci Application mocks from #7961 * mockery2 * Fixed new PrepareProposal test cases in state/execution_test.go * Fixed returned errors in consensus/state.go * lint * Addressed @cmwaters' comment Co-authored-by: mconcat <monoidconcat@gmail.com> Co-authored-by: JayT106 <JayT106@users.noreply.github.com> Co-authored-by: Sam Kleinman <garen@tychoish.com> Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
This is the core of the logic update for
PrepareProposalto match the specification.CreateProposalBlockno longer returns the block'sPartSet. This has been keptmockerytype for the mempool, replacing the hand written one. This has been keptFinalizeBlock. Will probably be useful when backportingFinalizeBlock-related codePrepareProposallogic in thepersistent_kvstoreapplicationProcessProposal, but there were two infra improvements in it that we needed here(on PR checklist below: it will be done at the time the feature branch is merged with
main, which will happen soon)PR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed