Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

TestContext sets tx nonces incorrectly (sometimes) #1461

Closed
ed255 opened this issue Jun 7, 2023 · 1 comment · Fixed by #1464
Closed

TestContext sets tx nonces incorrectly (sometimes) #1461

ed255 opened this issue Jun 7, 2023 · 1 comment · Fixed by #1464
Labels
good first issue Good for newcomers T-bug Type: bug

Comments

@ed255
Copy link
Member

ed255 commented Jun 7, 2023

The TestContext is used to setup an Ethereum state, run txs on it with go-ethereum and get the resulting traces. When using various transactions, the TestContext automatically assigns a default nonce to them, in sequence starting from 0 (and these values can then be overwritten by the user). This only works assuming all txs come from the same address and this address has nonce 0 at the beginning:

// By default, set the TxIndex and the Nonce values of the multiple transactions
// of the context correlative so that any Ok test passes by default.
// If the user decides to override these values, they'll then be set to whatever
// inputs were provided by the user.
transactions
.iter_mut()
.enumerate()
.skip(1)
.for_each(|(idx, tx)| {
let idx = u64::try_from(idx).expect("Unexpected idx conversion error");
tx.transaction_idx(idx).nonce(idx);
});

This automatic assignment of nonces doesn't work for example when you have 2 txs from different accounts (each with nonce = 0). I think the automatic assignment should only be implemented it if covers all cases, because otherwise the developer may take some time to realize what's failing: When having 2 txs from different accounts the developer may assume that each tx.nonce is 0 (because that's the default value of the MockTransaction), but they will find that the second tx is sent with nonce = 1.

So I think we should either remove this automatic (sometimes incorrect) nonce assignment, or alternatively do it right (by inspecting the from address, and checking the nonces in the setup accounts).

@ed255 ed255 added T-bug Type: bug good first issue Good for newcomers labels Jun 7, 2023
@zemse
Copy link
Member

zemse commented Jun 7, 2023

Thanks for marking this as good first issue. I am interested in taking this up.

@ChihChengLiang ChihChengLiang linked a pull request Jun 20, 2023 that will close this issue
4 tasks
ChihChengLiang pushed a commit that referenced this issue Jun 20, 2023
### Description

This PR changes the way TestContext sets tx nonces. Before nonce is set
serially. Applying this PR causes the tx to use nonce from the accounts
config.

### Issue Link


#1461

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update

### Contents

- Assigns correct nonce just after `func_tx` is called.
- Adds a test case to ensure that the nonces are assigned as expected.

### Rationale

The automatic assignment only works if all txs use same account and its
account nonce is zero. Otherwise dev has to manually override the nonce.
Inferring the nonce from account state adds convenience such that it
automatically works for same accounts as well as different accounts.

### How Has This Been Tested?

A test case has been added.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants