Validate transactions against local mempool state #338

charithabandi · 2023-10-04T22:58:59Z

List of changes:

ABCI App depends on the account store and accepts configurations such as WithoutGasCosts
ABCI maintains mempool state which gets reset per block. All transactions are validated against this mempool state. This weeds out any invalid txns at the mempool level and protects them from getting picked into the block
P2P hardening using CheckTx: Validates Tx payload, gas costs, nonces wrt local mempool state
ProcessProposal() checks for nonce ordering, gaps, duplicates, and continuity.
Fixed the error propagation by the Cancel() calls of Committer types
gas costs for validator operations is now managed by the Validator pkg

Gonna add integration tests, once the nonce override PR is pushed to the main.

pkg/abci/mempool.go

pkg/abci/abci.go

pkg/abci/mempool.go

pkg/abci/abci.go

pkg/abci/interfaces.go

pkg/abci/mempool.go

pkg/abci/abci.go

brennanjl · 2023-10-06T16:51:05Z

pkg/abci/abci.go

@@ -184,12 +205,72 @@ func (a *AbciApp) CheckTx(incoming abciTypes.RequestCheckTx) abciTypes.ResponseC
 		zap.String("sender", hex.EncodeToString(tx.Sender)),
 		zap.String("PayloadType", tx.Body.PayloadType.String()))

-	err = tx.Verify()
+	a.mempoolState.mu.Lock()
+	defer a.mempoolState.mu.Unlock()


I'm trying to think of a way to better abstract this. The other place that the mempool state gets locked is abstracted by a mempoolState method. I'd love to figure out how to do the same thing here, but I think it would require a larger change to better abstract mempool

pkg/abci/mempool.go

pkg/sessions/session.go

brennanjl · 2023-10-06T17:07:37Z

Still thinking a bit more, but it seems like we can maybe abstract away the mempool a bit better? This might not be possible, so if nothing comes to mind today then we will just continue as-is. But it just feels like the ABCI needs a lot of context on mempool, but mempool seems like it is really only account store specific, and could therefore be better abstracted

jchappelow

Just a couple quick comments that pertain to active discussions.
Still going through

pkg/abci/abci.go

pkg/abci/interfaces.go

pkg/abci/mempool.go

jchappelow

Looks to work as advertised, but have comments.

The one explicit panic is making me think though. Like why the method checks the map when it can just update the values in the *userAccount that reference the map entry that we just pulled.

Some thought via hacking a diff (don't cherry-pick directly, just consider pls): ff28bad

pkg/abci/abci.go

internal/app/kwild/server/build.go

Yaiba · 2023-10-09T21:22:13Z

pkg/abci/abci.go

@@ -193,33 +208,46 @@ func (a *AbciApp) BeginBlock(req abciTypes.RequestBeginBlock) abciTypes.Response
 func (a *AbciApp) CheckTx(incoming abciTypes.RequestCheckTx) abciTypes.ResponseCheckTx {
 	logger := a.log.With(zap.String("stage", "ABCI CheckTx"))
 	logger.Debug("check tx")
+	ctx := context.Background()


Just an observation.
It seems all AbciApp.*Tx methods, won't provide us a context through parameter, so it's really to us to manage context.
And all our abci modules methods receive Context.
So it's bit awkward to see in those AbciApp.*Tx methods, we just created a context.Background() context, without any kind of context management, I guess we'll add later ?

Yeah, we should manage our own contexts. Will raise an Issue on it

This is another issue that goes away in cometbft v0.38. https://github.com/kwilteam/kwil-db/pull/310/files#diff-189346089c5af5194675f23ce4b8b6c0104c41948bb052ec2850be3224429481

This is genuinely good, it feels right ABCI call the app with context

pkg/abci/abci.go

pkg/abci/abci_test.go

pkg/abci/mempool.go

Yaiba

lgtm

jchappelow · 2023-10-10T16:05:07Z

pkg/sessions/sql-session/session.go

-	if len(errs) > 0 {
-		s.log.Error("errors while cancelling session", zap.Error(errors.Join(errs...)))
+	if errs != nil {
+		s.log.Error("errors while cancelling session", zap.Error(errs))


OK to leave, but if you return an error you don't also log it.

pkg/sessions/session.go

jchappelow · 2023-10-10T16:07:50Z

pkg/modules/validators/transactions.go

 	if err != nil {
-		return nil, err
+		return resp(price), err


Why return a non-nil *ExecutionResponse if error is non-nil?

I had this doubt too, but in databases we seem to be returning execution response with tx price even during the errored scenarios. It's kinda unclear right now on the correct behavior as we don't have gas. Even with gas, what should be the tx fee on failed transactions?

* added extensible auth using build tags * go mod tidied * made gavins requested changes

charithabandi force-pushed the mempool branch from a17ebbf to fee7015 Compare October 4, 2023 23:04

This was linked to issues Oct 5, 2023

mempool needs more acceptance checks #329

Closed

block proposal and approval must ensure nonce continuity #341

Closed

charithabandi force-pushed the mempool branch 2 times, most recently from 1bac240 to 2ca2725 Compare October 5, 2023 21:00

charithabandi mentioned this pull request Oct 5, 2023

abci: nonce-ordering of transactions in proposed block #334

Merged

charithabandi force-pushed the mempool branch 3 times, most recently from d9d70d0 to cff3d44 Compare October 5, 2023 22:00

charithabandi marked this pull request as ready for review October 5, 2023 22:07

charithabandi force-pushed the mempool branch from cff3d44 to 1840544 Compare October 5, 2023 22:35

charithabandi commented Oct 5, 2023

View reviewed changes

pkg/abci/mempool.go Show resolved Hide resolved

charithabandi force-pushed the mempool branch from 1840544 to f5551b4 Compare October 6, 2023 14:23

Yaiba requested changes Oct 6, 2023

View reviewed changes

brennanjl reviewed Oct 6, 2023

View reviewed changes

jchappelow reviewed Oct 6, 2023

View reviewed changes

pkg/abci/abci.go Outdated Show resolved Hide resolved

pkg/abci/interfaces.go Show resolved Hide resolved

pkg/abci/mempool.go Outdated Show resolved Hide resolved

jchappelow requested changes Oct 6, 2023

View reviewed changes

pkg/abci/abci.go Outdated Show resolved Hide resolved

pkg/abci/abci.go Outdated Show resolved Hide resolved

pkg/abci/abci.go Outdated Show resolved Hide resolved

pkg/abci/abci.go Outdated Show resolved Hide resolved

charithabandi force-pushed the mempool branch 12 times, most recently from 6147f31 to 8cefede Compare October 9, 2023 20:07

charithabandi force-pushed the mempool branch from 8cefede to 023d300 Compare October 9, 2023 20:26

charithabandi requested review from brennanjl, jchappelow and Yaiba October 9, 2023 20:28

Yaiba reviewed Oct 9, 2023

View reviewed changes

charithabandi force-pushed the mempool branch from 023d300 to a3089ca Compare October 9, 2023 22:38

Yaiba approved these changes Oct 10, 2023

View reviewed changes

brennanjl approved these changes Oct 10, 2023

View reviewed changes

brennanjl force-pushed the mempool branch from a3089ca to ac422aa Compare October 10, 2023 15:34

jchappelow reviewed Oct 10, 2023

View reviewed changes

charithabandi force-pushed the mempool branch 2 times, most recently from a5a441f to 236c0c5 Compare October 10, 2023 20:00

validate uncommitted transactions against the mempool state

66b164f

* added extensible auth using build tags * go mod tidied * made gavins requested changes

charithabandi force-pushed the mempool branch from 236c0c5 to 66b164f Compare October 10, 2023 20:00

jchappelow approved these changes Oct 11, 2023

View reviewed changes

jchappelow merged commit 3b1edb2 into main Oct 11, 2023
2 checks passed

jchappelow deleted the mempool branch October 11, 2023 15:12

jchappelow added this to the v0.6.0 milestone Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate transactions against local mempool state #338

Validate transactions against local mempool state #338

charithabandi commented Oct 4, 2023 •

edited

Loading

brennanjl Oct 6, 2023

brennanjl commented Oct 6, 2023

jchappelow left a comment

jchappelow left a comment

Yaiba Oct 9, 2023

charithabandi Oct 9, 2023

jchappelow Oct 9, 2023

Yaiba Oct 9, 2023

Yaiba left a comment

jchappelow Oct 10, 2023

jchappelow Oct 10, 2023

charithabandi Oct 10, 2023

Validate transactions against local mempool state #338

Validate transactions against local mempool state #338

Conversation

charithabandi commented Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

brennanjl commented Oct 6, 2023

jchappelow left a comment

Choose a reason for hiding this comment

jchappelow 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

Yaiba 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

charithabandi commented Oct 4, 2023 •

edited

Loading