Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Initialize New Accounts to Block Number #8822

Closed
wants to merge 8 commits into from

Conversation

kevlu93
Copy link

@kevlu93 kevlu93 commented May 15, 2021

Related to paritytech/polkadot-sdk#326
This pull request changes the behavior of inc_account_nonce so that if the account nonce is zero (no transactions have been made yet), then the account nonce is set to the block number. Otherwise, the account nonce is incremented by one. In order to convert the block number to the Index type, the block number is first converted into a u32 type, as according to Substrate docs block numbers will typically be u32. The nonce is then derived from this number. Please note that the existing documentation on Substrate describing account initialization will need to be adjusted, as we should now mention that nonce will be initialized to the block number, not 0.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented May 15, 2021

User @kevlu93, please sign the CLA here.

@kevlu93 kevlu93 changed the title Klu init nonce to block Initialize New Accounts to Block Number May 15, 2021
@kevlu93 kevlu93 requested a review from shawntabrizi May 17, 2021 19:23
@shawntabrizi shawntabrizi added B7-runtimenoteworthy D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited C1-low PR touches the given topic and has a low impact on builders. labels May 18, 2021
@xlc
Copy link
Contributor

xlc commented May 18, 2021

This will break local nonce tracker for new accounts so is a breaking change that needs to be communicated to everyone

@shawntabrizi
Copy link
Member

This will break local nonce tracker for new accounts so is a breaking change that needs to be communicated to everyone

Added some tags here. Certainly something we can keep on the backburner until we push multiple other breaking changes.

@kirushik
Copy link

What would happen in case of chain reorgs?
It might be that on a different fork (till the state is finalized) the initialized nonce would have a different value, invalidating some pre-constructed transactions (or even worse, making transaction N+1 valid, while discarding N).

I would suggest to round the nonce here to tens/hundreds (probably (block-1)/100*100 or something — so we still have the safety margin even if the current block is 100000 exactly), to make it more stable in such cases.

@joepetrowski
Copy link
Contributor

I would suggest to round the nonce here to tens/hundreds (probably (block-1)/100*100 or something — so we still have the safety margin even if the current block is 100000 exactly), to make it more stable in such cases.

But this maps multiple block numbers to the same starting nonce, which pretty much makes this change pointless.

@xlc
Copy link
Contributor

xlc commented May 18, 2021

What would happen in case of chain reorgs?
It might be that on a different fork (till the state is finalized) the initialized nonce would have a different value, invalidating some pre-constructed transactions (or even worse, making transaction N+1 valid, while discarding N).

I would suggest to round the nonce here to tens/hundreds (probably (block-1)/100*100 or something — so we still have the safety margin even if the current block is 100000 exactly), to make it more stable in such cases.

There just no way to 100% solve this issue. So the nonce track have to give up if it see a tx with nonce 0. It have to wait until the transaction confirms before start tracking new nonce. This just won't work nice in an air-gapped environment. There are always trade-offs we need to make, do we want this edgecase when nonce is 0, or the other edgecase when an account is wiped?

@shawntabrizi
Copy link
Member

@kirushik yeah, your problem scenario would only ever apply to the very first transaction, and then queuing a bunch of subsequent ones after that.

Basically it can be avoided by performing at least one transaction on the account, and then you know your nonce starting point, and then queuing from there is trivial.

@viniul viniul added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels May 19, 2021
@JoshOrndorff
Copy link
Contributor

This is insecure even without re-orgs. Consider this scenario where an honest user alice gets her funds stolen.

Block N
Alice receives 100 tokens. The first activity ever on this account.
Her nonce is set to N.

Block N+1
Alice sends 10 tokens each to her 10 friends
Her nonce increases from N up to N+10
Her balance reaches zero
Her account is reaped

Block N+2
Alice receives 100 tokens
Her nonce is set to N+2

Block N+3
One of her "friends", the third one she sent the payment to, replays the payment transaction.

If the point is that this is better than the current initialize-to-zero approach, not that it's perfect. I agree with that.

You could make it even better by initializing to block_number * 100. Or maybe calculate the largest number of transactions that could fit in a block.

@emostov
Copy link
Contributor

emostov commented Jun 6, 2021

You could make it even better by initializing to block_number * 100

This idea sounds helpful for accounts that expect their number of txs to grow quicker than block height prior to being reaped. However, its worth noting this would exponentially reduce the window that we could use this setup for creating nonces. For example, u32::MAX == 4294967295, which with 6 second block times is ~817 years. But if we use a multiplier like 100, we are down to just ~8.17 years where we can use this setup. That being said, perhaps a happy medium is exposing a configurable multiplier to allow governance to tweak based on community needs.

@xlc
Copy link
Contributor

xlc commented Jun 7, 2021

I will suggest make this optional.

@emostov
Copy link
Contributor

emostov commented Jun 7, 2021

I will suggest make this optional.

If a multiplier is a storage item or configurable type for the pallet, you could set it to 1 to opt out.

I am not sure if substrate allows, but maybe you could even allow for a callback parameter that takes the block number as the input and outputs the nonce. That way the chain builder can do whatever they like without forking: always to 0, set to block num * multiplier, use a random number etc.

@shawntabrizi
Copy link
Member

Probably what we want to do is actually complete the migration of accounts out of FRAME system:

https://github.com/paritytech/substrate/pull/8254/files

Then we can design many hooks or even allow other developers to build their own account system with whatever logic they want.

@stale
Copy link

stale bot commented Jul 8, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 8, 2021
@stale stale bot closed this Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited D2-breaksapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants