`Tx` expiration #1123

grarco · 2023-02-07T12:02:32Z

Closes #1009

Based on #1106

Adds an optional expiration field to struct Tx to prevent transactions from being applied indefinitely.

Adds expiration field to struct Tx
Adds mempool_validate check against last committed block time
Adds process_proposal check against the proposed block time or, if missing, the last committed block time
Adds prepare_proposal check against the proposed block time (if missing accept txs as assumed to have been checked by mempool_validate)
Unit tests
Updates client to allow for a new expiration optional arg

grarco · 2023-02-07T13:06:10Z

pls update wasm

batconjurer · 2023-03-08T09:09:28Z

apps/src/lib/node/ledger/shell/mod.rs

@@ -129,6 +132,11 @@ pub enum ErrorCodes {
    InvalidOrder = 4,
    ExtraTxs = 5,
    Undecryptable = 6,
+    ReplayTx = 7,


This is unfortunately not documented in main, but the idea of these numbers is that past a certain threshold, we reject the block. In this branch, those are error codes >=4. However, we can't reject an entire block if we get the InvalidDecryptedChainId error since we have to include all decrypted txs in order. So its number should be lowered below the critical threshold

I think the same is also true of ExpiredDecryptedTx

apps/src/lib/node/ledger/shell/prepare_proposal.rs

batconjurer · 2023-03-08T09:14:57Z

apps/src/lib/node/ledger/shell/process_proposal.rs

-            status: if tx_results.iter().any(|res| res.code > 3) {
-                ProposalStatus::Reject as i32
-            } else {
+            status: if tx_results.iter().all(|res| {


Ah here. If the ordering of the errors is fixed, we just need to change the 3 to a 5 in the old code

I think I'm missing something, what do you mean when you say "in the old code"?

status: if tx_results.iter().any(|res| res.code > 3) { ProposalStatus::Reject as i32 } else { ```

Ah, I think I see what you mean. I've changed that code to use the enum variants because I was struggling to keep track of the error codes and thought that using the variants was more readable. Would you prefer to revert to using the codes instead?

We rely on it heaving in eth-bridge-integration, so I thinks it's easier for you to make this change than us.

We also document it a lot better and add helper functions, so it's not such a secret design choice anymore.

Actually, in this PR I've also changed process_proposal to reject the block in more cases: in particular to reject a block even in case of an InvalidTx or InvalidSig which we were not doing before. Does this look ok to you? In theory, the block proposer should be able to verify these at block construction time. So in essence the threshold could stay at 3 like before if I reorder the variants correctly in the enum

Yep, sounds good

batconjurer · 2023-03-08T09:15:51Z

apps/src/lib/node/ledger/shell/process_proposal.rs

+                                                code: ErrorCodes::ExpiredDecryptedTx
+                                                    .into(),
+                                                info: format!(
+                    "Dercrypted tx expired at {:#?}, block time: {:#?}",


Oh god. What did fmt do here??

Also, Dercrypted. 😆

* grarco/tx-lifetime: changelog: add #1123 [ci] wasm checksums update Misc adjustments Refactors `prepare_proposal` tx validation Refactors block time retrieval Clippy + fmt Tx expiration check in `prepare_proposal`. Unit test Improves tx expiration checks. Adds unit tests Tx expiration validation Updates client for tx expiration Adds `expiration` field to `Tx` Wrapper `epoch` in replay protection specs

…::setup`

grarco added a commit that referenced this pull request Feb 7, 2023

changelog: add #1123

ee06465

grarco force-pushed the grarco/tx-lifetime branch from ffde19d to 9cfb5cf Compare February 7, 2023 15:57

grarco added a commit that referenced this pull request Feb 8, 2023

changelog: add #1123

885e960

grarco force-pushed the grarco/tx-lifetime branch from 9cfb5cf to 7545109 Compare February 8, 2023 14:02

grarco mentioned this pull request Feb 8, 2023

Replay protection #1009

Closed

3 tasks

grarco marked this pull request as ready for review February 8, 2023 14:39

grarco requested a review from tzemanovic February 8, 2023 14:39

tzemanovic and others added 22 commits February 10, 2023 10:10

core: added TempWlStorage for ABCI++ prepare/process proposal

1368c19

changelog: add #1051

2bb8ead

Updates replay protection specs

0b1aa74

Adds replay protection internal address and vp

77e1627

Updates replay protections specs with governance and unsigned inner hash

e028f9f

Adds tx hash check in mempool validate

14019da

Wrapper commit hash on unsigned inner tx

c8710d7

Unit test mempool_validate

78f1333

Refactors unsigned_hash_tx

a5962d1

Fixes replay protection specs

8cbccf7

Replay protection checks in process_proposal

5decfc9

Refactors process_proposal

c795dce

Fixes error codes

c00cb14

Removes tx hash from storage in finalize_block

b592344

Updates process_proposal unit tests

f720b0a

Updates replay protection specs with protocol txs

a764bdb

Fixes finalize_block and adds unit test

1f83434

Updates process_proposal unit tests

ada51c6

Fmt

15eaf8d

Clippy

fbb2bf5

[ci] wasm checksums update

ef04e1c

changelog: add #1017

ec6a570

grarco requested a review from tzemanovic March 1, 2023 18:38

tzemanovic previously approved these changes Mar 7, 2023

View reviewed changes

batconjurer self-requested a review March 8, 2023 08:49

batconjurer requested changes Mar 8, 2023

View reviewed changes

grarco added a commit that referenced this pull request Mar 9, 2023

changelog: add #1123

29d953d

grarco dismissed tzemanovic’s stale review via 29d953d March 9, 2023 16:32

grarco force-pushed the grarco/tx-lifetime branch from 135f68c to 29d953d Compare March 9, 2023 16:32

grarco added a commit that referenced this pull request Mar 9, 2023

changelog: add #1123

283868c

grarco force-pushed the grarco/tx-lifetime branch from 29d953d to 283868c Compare March 9, 2023 18:24

grarco added a commit that referenced this pull request Mar 9, 2023

changelog: add #1123

0e5e6f0

grarco force-pushed the grarco/tx-lifetime branch from 283868c to 0e5e6f0 Compare March 9, 2023 18:53

grarco and others added 3 commits March 10, 2023 02:02

Misc adjustments

e30bd99

[ci] wasm checksums update

a11a3d1

changelog: add #1123

76ad54b

grarco force-pushed the grarco/tx-lifetime branch from 0e5e6f0 to 76ad54b Compare March 10, 2023 01:02

grarco requested a review from batconjurer March 10, 2023 10:15

batconjurer approved these changes Mar 11, 2023

View reviewed changes

tzemanovic approved these changes Mar 13, 2023

View reviewed changes

grarco mentioned this pull request Mar 22, 2023

Whitelist gas accounting #1242

Closed

grarco added a commit that referenced this pull request Mar 23, 2023

changelog: add #1123

06a7c2b

tzemanovic mentioned this pull request Mar 27, 2023

Restructure Namada Transactions #1255

Closed

grarco added a commit that referenced this pull request Mar 31, 2023

changelog: add #1123

ce71da8

tzemanovic added a commit that referenced this pull request Apr 7, 2023

evil: #1123 - update tests from TestShell:new to test_utils::setup

eb945ae

tzemanovic mentioned this pull request Apr 7, 2023

draft v0.15.x #1269

Closed

tzemanovic added the merged to draft label Apr 7, 2023

tzemanovic added a commit that referenced this pull request Apr 11, 2023

fixup! evil: #1123 - update tests from TestShell:new to `test_utils…

b4c4cfa

…::setup`

juped merged commit 403071e into main Apr 13, 2023

juped deleted the grarco/tx-lifetime branch April 13, 2023 06:07

bengtlofgren pushed a commit that referenced this pull request May 11, 2023

changelog: add #1123

d1650dd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Tx` expiration #1123

`Tx` expiration #1123

grarco commented Feb 7, 2023 •

edited

Loading

grarco commented Feb 7, 2023

batconjurer Mar 8, 2023

batconjurer Mar 8, 2023

batconjurer Mar 8, 2023

grarco Mar 8, 2023

batconjurer Mar 8, 2023 •

edited

Loading

grarco Mar 8, 2023

batconjurer Mar 9, 2023

batconjurer Mar 9, 2023

grarco Mar 9, 2023

batconjurer Mar 10, 2023

batconjurer Mar 8, 2023

batconjurer Mar 8, 2023

Tx expiration #1123

Tx expiration #1123

Conversation

grarco commented Feb 7, 2023 • edited Loading

grarco commented Feb 7, 2023

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

batconjurer Mar 8, 2023 • edited Loading

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

`Tx` expiration #1123

`Tx` expiration #1123

grarco commented Feb 7, 2023 •

edited

Loading

batconjurer Mar 8, 2023 •

edited

Loading