Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider non-zero default nonce #248

Open
ggwpez opened this issue Mar 19, 2024 · 15 comments
Open

Consider non-zero default nonce #248

ggwpez opened this issue Mar 19, 2024 · 15 comments

Comments

@ggwpez
Copy link
Member

ggwpez commented Mar 19, 2024

New or resurrected accounts use Nonce::default() that returns 0 in the Polkadot runtime.
We could change this to return the current block number, to avoid replay of immortal transactions.

Any downsides? Yea, its slower but nothing else i think.

@bkchr
Copy link
Contributor

bkchr commented Mar 25, 2024

Yeah sounds good. We can include it in 1.3.0.

@bkchr bkchr mentioned this issue Mar 25, 2024
9 tasks
@kianenigma
Copy link
Contributor

and once paritytech/polkadot-sdk#1557 is merged and release I suppose?

@ggwpez
Copy link
Member Author

ggwpez commented Apr 1, 2024

and once paritytech/polkadot-sdk#1557 is merged and release I suppose?

We can do it without that actually.

@kianenigma
Copy link
Contributor

New or resurrected accounts use Nonce::default() that returns 0 in the Polkadot runtime.
We could change this to return the current block number, to avoid replay of immortal transactions.

Ah so you are suggesting a newtype for nonce with custom Default impl? yes, makes sense.

@xlc
Copy link
Contributor

xlc commented Apr 12, 2024

This will confuse block explorer and indexer

@bkchr
Copy link
Contributor

bkchr commented Apr 12, 2024

This will confuse block explorer and indexer

Why? I think the problem is bigger for wallets.

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue May 6, 2024
Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able
to provide a custom default for any type. This can, then, be used to
provide the nonce type that returns the current block number as the
default, to avoid replay of immortal transactions.

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this issue Jun 5, 2024
Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able
to provide a custom default for any type. This can, then, be used to
provide the nonce type that returns the current block number as the
default, to avoid replay of immortal transactions.

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@bkchr
Copy link
Contributor

bkchr commented Jul 26, 2024

@ggwpez do you still want to tackle this?

TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Needed for: polkadot-fellows/runtimes#248

This PR introduces a new type `TypeWithDefault<T, D: Get<T>>` to be able
to provide a custom default for any type. This can, then, be used to
provide the nonce type that returns the current block number as the
default, to avoid replay of immortal transactions.

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@athei
Copy link
Contributor

athei commented Nov 4, 2024

This will confuse block explorer and indexer

Why? I think the problem is bigger for wallets.

If it is a problem at all.

Having this functionality will become more important when adding Ethereum compatibility: Eth transactions are immortal.

@kianenigma
Copy link
Contributor

We should do this. the Type added in https://github.com/paritytech/polkadot-sdk/pull/4034/files#r1829220430 has an issue that it derives the default TypeInfo, which as said will confuse the explorers. We need a similar custom type, but with a manual TypeInfo impl that would keep it identical in the metadata. Then no explorer or wallet would have any issues.

@bkchr
Copy link
Contributor

bkchr commented Nov 5, 2024

Having this functionality will become more important when adding Ethereum compatibility: Eth transactions are immortal.

What? We are going to inherit the ETH transaction format? Why? Please don't say "metamask".

@athei
Copy link
Contributor

athei commented Nov 6, 2024

In order to support Ethereum wallets you need to accept their transactions. This what they sign. Of course we can build our own transaction format but that won't make existing Dapps compatible. See, I didn't even say the M word.

@bkchr
Copy link
Contributor

bkchr commented Nov 6, 2024

But I thought for this snaps exists? So you can extend the big M?

So, we will only support their tx? But will not have a ETH compatible state root etc or?

@athei
Copy link
Contributor

athei commented Nov 6, 2024

But I thought for this snaps exists?

Requirement is that it works without having the user to install anything. Also, it would be another thing to maintain. We might as well just support it as-is. Both frontier and Acala are doing that, too. We try to be as compatible as possible.

So, we will only support their tx?

No. You can still call pallet_revive via normal extrinsic. Both is supported. However, Dapps written against any Ethereum javascript library will submit Ethereum style transactions. There is nothing we can do about it.

Theoretically, we could require a custom transaction format and have that signed by Metamask using https://eips.ethereum.org/EIPS/eip-712. However, supporting the legacy transaction formats makes sure that it works everywhere. Without that it would mean that existing javascript libraries work as is. A whole lot more work for us to provide wrappers for all of them. Apart from the missing mortality I don't see a big reason why not to. Especially if we can initialize the nonce to the block number as proposed here.

So, we will only support their tx? But will not have a ETH compatible state root etc or?

Yeah seems to be working for frontier and Acala. We just return our state root when somebody requests a block. Dapps just seem to be using this as identifiers if at all.

https://docs.metamask.io/wallet/reference/json-rpc-methods/eth_getblockbyhash/
https://github.com/paritytech/polkadot-sdk/blob/a4791617241732f4fecafe21e12c16b7fe42f73e/substrate/frame/revive/rpc/src/client.rs?plain=1#L747-L773

Its not like there are any EVM light clients in production that would try to use them to verify a proof.

@kianenigma
Copy link
Contributor

@ggwpez do you still want to tackle this?

Proposed this ticket to @re-gius. My suggestion to him, as noted above, is to not use the TypeWithDefault and instead use a custom new type with a manual implementation of TypeInfo. This should avoid any issues while decoding the nonce; from the outer world, the nonce is still u64.

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Nov 26, 2024
See polkadot-fellows/runtimes#248 : using
`TypeWithDefault` having derived `TypeInfo` for `Nonce` causes a
breaking change in metadata for nonce type because it's no longer `u64`.

Adding a default implementation of `TypeInfo` for `TypeWithDefault` to
restore the original type info in metadata.

---------

Co-authored-by: Kian Paimani <[email protected]>
@re-gius
Copy link

re-gius commented Nov 27, 2024

paritytech/polkadot-sdk#6562 should have fixed the Type Info. Now we need to wait for the next release, and then update the fellowship repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants