feat!: Add a block validity rule that all PFB transactions must have valid signatures #1300

evan-forbes · 2023-01-26T06:13:23Z

Overview

This PR adds a signature check in ProcessProposal for PFB transactions.

blocked by celestiaorg/cosmos-sdk#296

closes #979

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

codecov-commenter · 2023-01-26T07:15:21Z

Codecov Report

Attention: Patch coverage is 0% with 94 lines in your changes missing coverage. Please review.

Project coverage is 48.17%. Comparing base (0b75c9c) to head (7f157b9).
Report is 1155 commits behind head on main.

Files with missing lines	Patch %	Lines
app/parse_txs.go	0.00%	39 Missing ⚠️
app/process_proposal.go	0.00%	27 Missing ⚠️
app/verify_txs.go	0.00%	16 Missing ⚠️
app/prepare_proposal.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1300      +/-   ##
==========================================
- Coverage   49.20%   48.17%   -1.04%     
==========================================
  Files          76       77       +1     
  Lines        4292     4384      +92     
==========================================
  Hits         2112     2112              
- Misses       2004     2096      +92     
  Partials      176      176

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

app/test/fuzz_abci_test.go

go.mod

evan-forbes · 2023-01-26T07:16:02Z

testutil/manual_blobtx_gen.go

+// RandBlobTxsWithAccounts will create random blob transactions using the
+// provided configuration. The account info is queried directly from the
+// application. One blob transaction is generated per account provided.
+func RandBlobTxsWithAccounts(


adding to our testing technical debt a bit by continuing to copy paste minor alterations of this function. #1114

we have to keep this in the testutil pkg instead of blob factory because we're importing the app to query directly, and don't want to cause an import cycle in blobfactory

cmwaters

Everything here makes sense to me.

Valid signatures don't mean valid PFBs. I'm not sure how important the distinction is for the users that requested this. But just want to make sure that they're aware that although all PFBs have valid signatures they may be missing other things.

evan-forbes · 2023-01-26T09:38:51Z

Valid signatures don't mean valid PFBs. I'm not sure how important the distinction is for the users that requested this. But just want to make sure that they're aware that although all PFBs have valid signatures they may be missing other things.

yes good point this is definitely true. In this case, we only want to be able to make an honest majority assumption that the signatures are valid, because it dramatically reduces the proving time of some zk proofs.

This is only the minimum to close #979,

In practice this could mean that we only add an additional signature check, but we could also check that there are enough funds to pay gas...

if we want other features such as fee burning, that will essentially require executing the transactions in full before we progress

cmwaters · 2023-01-26T10:28:57Z

I believe the default implementation of the SDK v0.47 (i.e. with the introduction of PrepareProposal and ProcessProposal) by default calls something similar to CheckTx on all transactions in ProcessProposal

evan-forbes · 2023-01-26T11:16:32Z

to clarify things a bit, we are also already enforcing the validate basic checks, that only a single MsgPFB is in a PFB transaction, and that the share commitment is valid. Therefore, I believe the only other way an invalid PFB can be introduced is if the account does not have enough funds to pay for gas.

cmwaters · 2023-01-26T11:26:57Z

Therefore, I believe the only other way an invalid PFB can be introduced is if the account does not have enough funds to pay for gas

Or because of an incorrect nonce

evan-forbes · 2023-01-26T11:29:01Z

Or because of an incorrect nonce

I believe we are checking for that in this PR since that is required to verify the signature via the SigVerifyAnteDecorator

edit: link to save a search https://github.com/celestiaorg/cosmos-sdk/blob/472f7a7633c16835145381c4effc31dc8c99f738/x/auth/ante/sigverify.go#L267-L273

cmwaters · 2023-01-26T12:09:27Z

I believe we are checking for that in this PR since that is required to verify the signature via the SigVerifyAnteDecorator

I see, but is the cached state tracked and updated?

Take the following scenarios:

A user sends a vote message and a PFB with the same nonce number (5 for example). Upon execution only the first message will be executed. The second will be invalidated by the execution of the first. However both would pass CheckTx.
A user sends a vote message and a PFB message with incrementing nonce numbers (i.e. 5 and 6). In that order both should be able to be executed in the same block. But if we check from a snapshot of state, the second message with a nonce of 6 will be invalid.

…and Process Proposal

…res in prepare proposal

…ross the board

evan-forbes

most of the changes introduced here are testing related since we added stateful requirements to both prepare and process proposal, and many of the test txs we were using before didn't have valid signatures or nonce usage.

evan-forbes · 2023-01-29T02:01:42Z

app/prepare_proposal.go

+	sdkCtx, err := app.NewProcessProposalQueryContext()
+	if err != nil {
+		panic(err)
+	}
+
+	// increment the sequences of the standard cosmos-sdk transactions. Panics
+	// from the anteHandler are caught and logged.
+	isHandler := incrementSequenceAnteHandler(&app.AccountKeeper)
+	normalTxs, sdkCtx = filterStdTxs(app.Logger(), app.txConfig.TxDecoder(), sdkCtx, isHandler, normalTxs)
+
+	// check the signatures and increment the sequences of the blob transations,
+	// and filter out any that fail. Panics from the anteHandler are caught and
+	// logged.
+	svHandler := sigVerifyAnteHandler(&app.AccountKeeper, app.txConfig)
+	blobTxs, _ = filterBlobTxs(app.Logger(), app.txConfig.TxDecoder(), sdkCtx, svHandler, blobTxs)


here is where we add stateful checks to prepare proposal. notice that we are incrementing the nonce of the normal txs first before we are checking the signatures of the PFB txs.

evan-forbes · 2023-01-29T02:05:17Z

app/test/fuzz_abci_test.go

-		{"many small single share single blob transactions", 10000, 1, 400},
+		{"many small single share single blob transactions", 1000, 1, 400},


reduced the length of this first test because it takes so long that sometimes we don't even run it once. changing this means that, at least in this test, are no longer testing for the weird things that can happen when using a bunch of small PFBs occasionally. We might still want to do that in the future, but I'm less worried now that we are limiting the number of txs to 5000.

evan-forbes · 2023-01-29T02:06:18Z

app/test/fuzz_abci_test.go

+					// create 100 send transactions
+					sendTxs := testutil.SendTxsWithAccounts(
+						t,
+						testApp,
+						encConf.TxConfig.TxEncoder(),
+						kr,
+						1000,
+						accounts[0],
+						accounts[len(accounts)-100:],
+						"",
+					)


this refactor was needed cause now we need access to state to query for account info before we can create the txs

evan-forbes · 2023-01-29T02:08:24Z

app/test/prepare_proposal_test.go

-		// verify the signatures of the prepared txs
-		sdata, err := signer.GetSignerData()
-		require.NoError(t, err)
-
-		dec := encoding.IndexWrapperDecoder(encCfg.TxConfig.TxDecoder())
-		for _, tx := range res.BlockData.Txs {


this portion of this test was leftover from when we were malleating txs for multiple square sizes, so we can remove. It no longer works since this portion of the test only works when we don't need access to state.

evan-forbes · 2023-01-29T02:09:36Z

app/test/prepare_proposal_test.go

+	accnts := testfactory.GenerateAccounts(numBlobTxs + numNormalTxs)
+	testApp, kr := testutil.SetupTestAppWithGenesisValSet(accnts...)


all of our prepare/process proposal tests need to sign txs with correct nonces and account numbers, therefore we have to change most of the tests to account for this

evan-forbes · 2023-01-29T02:12:55Z

app/test/process_proposal_test.go

@@ -154,59 +270,6 @@ func TestProcessProposal(t *testing.T) {
 	}
 }

-func TestProcessProposalWithParityShareNamespace(t *testing.T) {


moved this test up to the other test, see comment there to see why we probably don't even need this test unless we have some code that let's us bypass these checks in prepare proposal #233

evan-forbes · 2023-01-29T02:13:23Z

app/test/process_proposal_test.go

-	require.Equal(t, abci.ResponseProcessProposal_REJECT, res.Result)
-}
-
-func TestProcessProposalWithTamperedSequenceStart(t *testing.T) {


consolidated this test in the above test

app/test/process_proposal_test.go

evan-forbes · 2023-01-29T02:17:27Z

app/verify_txs.go

+// recoverHandler will simply wrap the caught panic in an error containing the
+// stack trace.
+func recoverHandler(recoveryObj interface{}) error {
+	return sdkerrors.Wrap(
+		sdkerrors.ErrPanic, fmt.Sprintf(
+			"recovered: %v\nstack:\n%v", recoveryObj, string(debug.Stack()),
+		),
+	)
+}


the nonce increment decorator panics if the account isn't present, so we have to add this. It should never get hit if block producers are honest and are running check tx

evan-forbes · 2023-01-29T02:19:40Z

testutil/direct_tx_gen.go

+// RandBlobTxsWithAccounts will create random blob transactions using the
+// provided configuration. The account info is queried directly from the
+// application. One blob transaction is generated per account provided.
+func RandBlobTxsWithAccounts(
+	t *testing.T,
+	capp *app.App,


We need to sign txs with valid sequences and account numbers, and have to query for those w/o grpc if we want to use the testapp, so I created even more testing functions (that we should consolidate in #1114) to query the app directly

rootulp

I don't have any blocking feedback. I do have a question: IIUC the motivation for this PR was to verify the validity of signatures on PFB transactions so that once a transaction is included in a block, we know that its signature was valid.

Now that prepare proposal and process proposal are verifying the sequence number and signature of PFB transactions, should we consider (at a later date) extending that to apply to all types of transactions?

app/parse_txs.go

app/prepare_proposal.go

app/test/prepare_proposal_test.go

Co-authored-by: Rootul P <[email protected]>

…valid signatures (#1300) ## Overview This PR adds a signature check in ProcessProposal for PFB transactions. blocked by celestiaorg/cosmos-sdk#296 closes #979 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords --------- Co-authored-by: Rootul P <[email protected]>

evan-forbes added 3 commits January 22, 2023 15:29

feat!: check signatures for PFB txs during processproposal

fe08277

feat: add new testing function to create txs using the app

f14f1c7

fix: update remaining tests and cache context

799a065

evan-forbes added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules block validity rule labels Jan 26, 2023

evan-forbes added this to the Incentivized Testnet milestone Jan 26, 2023

evan-forbes self-assigned this Jan 26, 2023

evan-forbes requested a review from cmwaters January 26, 2023 06:13

MSevey requested review from a team and MSevey and removed request for a team January 26, 2023 06:13

evan-forbes requested review from rootulp and removed request for MSevey January 26, 2023 06:13

evan-forbes added 2 commits January 26, 2023 07:23

chore: revert uneccessary test helper

b8b5235

chore: add extra unit test and clean up

53b0bd2

MSevey requested a review from a team January 26, 2023 06:59

Merge branch 'main' into evan/pfb-sig-check

827faa3

evan-forbes mentioned this pull request Jan 26, 2023

feat: add additonal validity checks during process proposal #1128

Closed

4 tasks

evan-forbes added the ABCI modifies an ABCI method label Jan 26, 2023

evan-forbes commented Jan 26, 2023

View reviewed changes

cmwaters previously approved these changes Jan 26, 2023

View reviewed changes

chore: use final version of the sdk

7c1fafe

evan-forbes dismissed cmwaters’s stale review via 7c1fafe January 26, 2023 12:10

evan-forbes added 9 commits January 27, 2023 14:35

feat!: account for multiple txs from the same signer in both Prepare …

7eae040

…and Process Proposal

chore: add appropriate state access now that we are verifying signatu…

520faf7

…res in prepare proposal

chore: add appropriate state to remaining prepare proposal tests

28cb6fa

feat!: catch panic when filtering txs

8c8239e

refactor: combine tests so that the old ones use the new required state

697bdd5

chore: add test for filtering in prepareproposal

4db2681

Merge branch 'main' into evan/pfb-sig-check

3af3536

chore: add a replace statement for nmt since we haven't downgraded ac…

9999b61

…ross the board

chore: update names

539f384

evan-forbes commented Jan 29, 2023

View reviewed changes

evan-forbes added 2 commits January 29, 2023 03:21

chore: linter

2db0ee9

chore: add test case to ensure panic is caught

8192fc0

evan-forbes marked this pull request as ready for review January 29, 2023 02:34

rootulp previously approved these changes Jan 29, 2023

View reviewed changes

app/parse_txs.go Outdated Show resolved Hide resolved

app/prepare_proposal.go Outdated Show resolved Hide resolved

app/test/prepare_proposal_test.go Outdated Show resolved Hide resolved

app/test/prepare_proposal_test.go Outdated Show resolved Hide resolved

chore: docs wording

f45d4a9

Co-authored-by: Rootul P <[email protected]>

evan-forbes dismissed rootulp’s stale review via f45d4a9 January 30, 2023 19:35

MSevey requested a review from a team January 30, 2023 19:35

evan-forbes and others added 4 commits January 30, 2023 13:36

chore: docs

47b36a8

Co-authored-by: Rootul P <[email protected]>

chore: use consistent naming for antehandlers

d508e9e

chore: use seq instead of is

c50e7b8

Co-authored-by: Rootul P <[email protected]>

Merge branch 'main' into evan/pfb-sig-check

b40bb7a

rootulp previously approved these changes Jan 30, 2023

View reviewed changes

Merge branch 'main' into evan/pfb-sig-check

7f157b9

evan-forbes dismissed rootulp’s stale review via 7f157b9 January 31, 2023 13:34

MSevey requested a review from a team January 31, 2023 13:34

rootulp approved these changes Jan 31, 2023

View reviewed changes

evan-forbes merged commit a6f6c23 into main Jan 31, 2023

evan-forbes deleted the evan/pfb-sig-check branch January 31, 2023 22:02

evan-forbes mentioned this pull request Feb 10, 2023

Investigate the tests race conditions #1369

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add a block validity rule that all PFB transactions must have valid signatures #1300

feat!: Add a block validity rule that all PFB transactions must have valid signatures #1300

evan-forbes commented Jan 26, 2023 •

edited

Loading

codecov-commenter commented Jan 26, 2023 •

edited

Loading

evan-forbes Jan 26, 2023

cmwaters left a comment

evan-forbes commented Jan 26, 2023

cmwaters commented Jan 26, 2023

evan-forbes commented Jan 26, 2023

cmwaters commented Jan 26, 2023

evan-forbes commented Jan 26, 2023 •

edited

Loading

cmwaters commented Jan 26, 2023

evan-forbes left a comment

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

evan-forbes Jan 29, 2023

rootulp left a comment

		{"many small single share single blob transactions", 10000, 1, 400},
		{"many small single share single blob transactions", 1000, 1, 400},

		accnts := testfactory.GenerateAccounts(numBlobTxs + numNormalTxs)
		testApp, kr := testutil.SetupTestAppWithGenesisValSet(accnts...)

feat!: Add a block validity rule that all PFB transactions must have valid signatures #1300

feat!: Add a block validity rule that all PFB transactions must have valid signatures #1300

Conversation

evan-forbes commented Jan 26, 2023 • edited Loading

Overview

Checklist

codecov-commenter commented Jan 26, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

evan-forbes commented Jan 26, 2023

cmwaters commented Jan 26, 2023

evan-forbes commented Jan 26, 2023

cmwaters commented Jan 26, 2023

evan-forbes commented Jan 26, 2023 • edited Loading

cmwaters commented Jan 26, 2023

evan-forbes 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

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

evan-forbes commented Jan 26, 2023 •

edited

Loading

codecov-commenter commented Jan 26, 2023 •

edited

Loading

evan-forbes commented Jan 26, 2023 •

edited

Loading