Implement PreprocessTxs #21
Conversation
|
I've skimmed through the code and it looks pretty complete and good 👍🏼 Can you track the changes you plan to incorporate in a list of todos in the opening comment? It would a reviewer understand what parts are going to change (if any).
If there are still some open design decisions, let's discuss them here, or in the corresponding issue. |
|
whoops, accidentally hit tab 3x + enter to close the PR... The first draft testing is at a "good-enough-to-start-reviewing" stage, but there will definitely need to be some additions in the near future. I'm going to mark this ready for review, and start working on the CI issues. Notes:
|
Codecov Report
@@ Coverage Diff @@
## master tendermint/spec#21 +/- ##
=========================================
Coverage ? 15.04%
=========================================
Files ? 14
Lines ? 1888
Branches ? 0
=========================================
Hits ? 284
Misses ? 1588
Partials ? 16 Continue to review full report at Codecov.
|
Yeah, maybe the package name can help to differentiate both? For example would
Sounds like this should be tracked in an issue! |
| Txs: processedTxs, | ||
| Messages: &core.Messages{MessagesList: shareMsgs}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Note that in PreProcess the app also needs to track the (original / mempool) Tx that were processed, s.t. on re-check it can tell ll-core to remove them from the mempool.
This has the caveat that currently only the proposer will call preprocess: the app needs to know if a proposed block didn't make it through consensus (e.g. because of timeouts) and other nodes to be able to evict their mempools too...
@adlerjohn you and I need to think about, too.
There was a problem hiding this comment.
Not necessarily? Conflicting transactions (which includes duplicates in the mempool) between the block and the mempool are removed with CheckTx, so PreProcess doesn't technically need to do that?
There was a problem hiding this comment.
Looks like I'm confused about this again, then 🤔
How does the app know which (mempool) Tx made it into the block if the app doesn't track this?
Sure, it "sees" the Tx during each deliver Tx but only the already processed which are different from the mempool Tx potenially.
There was a problem hiding this comment.
Leaving this here for reference (quoting @adlerjohn):
You don't need to know which txs from the mempool were included in a block, you only need to know which mempool transactions conflict with the ones in a block.
So running CheckTx of all txs in the mempool after you commit a block is sufficient.
If the tx in the mempool conflicts. It'll have the same nonce and sender.
So my concern was the mapping between mempool tx <-> block tx but as mentioned by John that mapping is essentially there via nonce and sender.
| // CreateCommit generates the commit bytes for a given message, namespace, and | ||
| // squaresize using a namespace merkle tree and the rules described at | ||
| // https://github.com/lazyledger/lazyledger-specs/blob/master/rationale/message_block_layout.md#non-interactive-default-rules | ||
| func CreateCommit(k uint64, namespace, message []byte) ([]byte, error) { |
|
For posterity, tendermint/spec#28 tracks removing redundant pubkey in |
|
I forgot to make a PR for the change needed in the cosmos-sdk that needs to be merged before this can be merged. |
fix bug left from sdk that stops proto-gen from working properly add access to the tx encoder and decoder add all the basic proto messages needed for PayForMessage run make protoc-gen
remove placeholder type that causes a compilation error lazyledgerapp: add pay for message to the handler added verification methods to payformessage connected payformessage to rest of sdk refactored preprocessing update the branch of the sdk that allows for running tx outside of DeliverTx proto: use the users public key instead their address keeper: remove burning fees verify signatures using public key added notes for next todo remove compile time error proto: remove enums that are not going to be used for the mvp proto: added tx type for pay for message lazyledgerapp/types: fleshed out and finalized PayForMessage types
delete unused utility func change the respone type name to appease the linter go mod tidy app: just save the encoding config instead of the only the tx encoder and decoders types: register PayForMessage types in the codec registry break up testing into two chunks. document process add a sanity check for signing and verifying signatures lazyledgerapp: sign the commit bytes instead of the message bytes add genesis accounts, and attempt to debug why the signature data is getting erased clean up tests a bit, and fix bug causing the inability to verify the signature app: add sorting by namespace ids for returned core.Messages clean up unused code slight reorg of tests unused code caught by linter only pass successfully processed txs update test fix accidental changes to go.mod mod.go: fixed the dep that was causing weird ci errors and failed proto generation init simapp flags when testing run make build
…entation, remove redundant ValidateBasicChecks payformessage: use constants and explanation of panic use namespace and share constants from ll-core app: added comments to constant copy and pasted from the sdk review feedback: switch to using constant for namespaceID size review feedback: change name of GetCommitSignBytes to GetCommitmentSignBytes review feedback: use the max square size from lazyledger-core instead of 64 fix doc typo Co-authored-by: John Adler <[email protected]> review feedback: docs update for the public key method switch to using constants in the docs too Update x/lazyledgerapp/types/payformessage.go Co-authored-by: Ismail Khoffi <[email protected]> switch to updated PR for cosmos-sdk change and update deps
…hare commitment instead of not allowing chains update failing test to new expected result now that the square size is 128 instead of 64
evan-forbes
force-pushed
the
evan/PayForMsg-from-scractch
branch
from
72ab5f9 to
a41e189
Compare
app/abci.go
Outdated
| // execute the tx in runTxModeDeliver mode (3) | ||
| // execution includes all validation checks burning fees | ||
| // currently, no fees are burned | ||
| _, _, err = app.BaseApp.TxRunner()(runTxModeDeliver, rawTx) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Actually, didn't we say to not execute the Tx immediately until we add explicit ABCI methods for that purpose?
See: https://github.com/tendermint/spec/issues/208#issuecomment-721158558
and tendermint/spec#254
Also, if run the Tx during preprocess, won't that app still try to execute the Tx from the previous block during, too?
There was a problem hiding this comment.
The sdk uses the runTx method to run stateful checks and execute any state changing transactions. While the runTxModeDeliver (signal to keep changes to the state) flag is being passed to runTx, the handler doesn't do anything (see below).
Also, if run the Tx during preprocess, won't that app still try to execute the Tx from the previous block during, too?
Under the hood, there are two distinct transactions, so we can specify which state needs to change during PreprocessTxs and which need to change during DeliverTx. There is MsgWirePayForMessage, which is passed to PreprocessTxs and is being pushed through runTx during PreprocessTxs. There's also SignedTransactionDataPayForMessage, which is derived from MsgWirePayForMessage as described in the spec, which is not being pushed through runTx until DeliverTx and is instead returned by PreprocessTxs. Each is ran separately, and each has its own signature.
The actual actions that occur during transaction execution are dictated in the module's Keeper. We can look at what the handler for MsgWirePayForMessage does here, which is nothing atm. There is not currently a handler for SignedTransactionDataPayForMessage, which will throw an error if it is ran later via DeliverTx. While throwing an error if actually, this shouldn't throw an error! I'll include a handler that doesn't do anything.SignedTransactionDataPayForMessage is passed to DeliverTx was intentional, I think having a handler for it that always throws an error would be more explicit.
This brings up that I should have better documentation for all of this "under-the-hood" stuff, even if some of it is temporary. Where would the best place for that be? I was planning on making a full blown integration test later #24, but this might need to be done in this PR.
There was a problem hiding this comment.
Yeah, the runTxModeDeliver sounds like it will actually deliver the Txs. That is what confused me. Thanks for clarifying.
This brings up that I should have better documentation for all of this "under-the-hood" stuff, even if some of it is temporary. Where would the best place for that be?
This brings us back to that ADR discussion. Maybe we should document this kind of decision in light-weight ADRs that either precede or accompany PRs. And yes, (integration) tests would be a great way to increase confidence that everything behaves as we expect.
There was a problem hiding this comment.
After talking with @evan-forbes in person, we've decided to drop the runTx call and rely soley on checkTx for now. We will still have to take care of this in the future but we can likely tackle this together with immediate execution (which, as far as I understand is required for fee burning anyway).
use the latest version the lazyledger/cosmos-sdk instead of the modified version
fix SignedTransactionDataPayForMessage generation so that padding is added to the message if necessary change name of CreateCommit to CreateCommitment
evan-forbes
force-pushed
the
evan/PayForMsg-from-scractch
branch
from
4690d63 to
6abdee4
Compare
|
a few things have been changed:
|
remove extra redundant check
evan-forbes
force-pushed
the
evan/PayForMsg-from-scractch
branch
from
7fb1277 to
7295885
Compare
Co-authored-by: Ismail Khoffi <[email protected]>
Description
This PR implements the new
PreprocessTxsABCI method, along with a refactor of theWirePayForMessagemessage thatPreprocessTxsacts upon.Notes: a slightly modified branch of the cosmos-sdk is required.
TODO
closes: tendermint/spec#22
docs/) or specification (x/<module>/spec/)Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes