Fix eth_getTransactionCount behavior for unknown acccounts

[AlexRamRam]

Return starting account nonce for unknown accounts instead of error

i.e. In Hardhat, the following should return the starting account nonce:
await ethers.provider.getTransactionCount(ethers.HDNodeWallet.createRandom().address)

[HalosGhost]

@AlexRamRam this looks like a really simple change to bring our EVM shim closer-in-line with how things are expected to work, but the code change isn't (at least to my eye) particularly self-explanatory. Is there a simple test that can be added to ensure we don't accidentally undo this behavior in the future, or a script you can provide which would give an easy route to a t-ACK?

[AlexRamRam]

@AlexRamRam this looks like a really simple change to bring our EVM shim closer-in-line with how things are expected to work, but the code change isn't (at least to my eye) particularly self-explanatory. Is there a simple test that can be added to ensure we don't accidentally undo this behavior in the future, or a script you can provide which would give an easy route to a t-ACK?

Running the one line code mentioned in the main comment in hardhat (connected to the opencbdc network) currently generates an "Internal error". With this change the returned value is the starting nonce. Please let me know if this is not clear.

[HalosGhost]

I understand that the change solves a problem you were running into; I mean, it's not clear from the diff itself why the change (which adds an || clause to an if-condition checking whether the second member of the iterator returned from .find() has .size() == 0) results in returning the starting account nonce.

In particular, adding this would seem to make any account for which the .size() is 0 return to_hex_trimmed(evmc::uint256be(1)); but that's the only change that's obvious. I would expect adding a positive || clause to an if conditional would only increase (or not change) the number of cases for which the then-branch of the conditional is taken. So, is the starting nonce of an unknown account always to_hex_trimmed(evmc::uint256be(1)) (I would assume not since “nonce” usually implies a lack of reuse, but I'm not particularly familiar with this instance.)

The second request is separate: we specifically want reviewers to be able to test changes before they get merged (either by vetting a test case added to the test suite, or by running through steps to reproduce). Should I plan to test this by running a hardhat environment connected to a parsec/evm network and executing the command above? Is there a test that could be easily added to the test suite to avoid regression and simplify testing instead?

[HalosGhost]

If indeed the “starting account nonce” should always be to_hex_trimmed(evmc::uint256be(1)), that's an easy clarification (and potentially a good opportunity to introduce a meaningful variable name in-place of a magic number, but that's a separate discussion).

[AlexRamRam]

A good definition of an account nonce and its relationship to transaction count is here:
nonce – A counter that indicates the number of transactions sent from an externally-owned account or the number of contracts created by a contract account. Only one transaction with a given nonce can be executed for each account, protecting against replay attacks where signed transactions are repeatedly broadcast and re-executed.

At the moment, PArSEC's transaction count starts at 1 (should really be 0) and hence the default transaction count for a new account is 1 just as the default balance for a new account is 0. There is an existing comment: // For accounts that don't exist yet, return 1

Great idea to add the unit test - will do this.

Please let me know if this addresses your question.

[HalosGhost]

Adding a unit test will definitely make a t-ACK easier, thank you!

And your clarification (that this is a “nonce” per-transaction-per-account) definitely answered my main question. 👍

[AlexRamRam]

Adding a unit test will definitely make a t-ACK easier, thank you!

And your clarification (that this is a “nonce” per-transaction-per-account) definitely answered my main question. 👍

Test added

[HalosGhost]

Simple change to ensure that newly-created accounts have their transaction-count set to 1, and a test to avoid accidental regressions. Looks great!