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

Add a basic version of the CheckMetadataHash signed extension #1590

Merged
merged 7 commits into from
May 23, 2024

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented May 17, 2024

This version doesn't try to hash the metadata and provide a hash at the moment, and just tells the node that there will be no hash via a 0 byte in the tx payload.

I tested this by compiling polkadot from paritytech/polkadot-sdk#4274 and running cargo run --example tx_basic with and without the new extension. It failed without and worked with.

Close #1605

@jsdw jsdw marked this pull request as ready for review May 17, 2024 16:32
@jsdw jsdw requested a review from a team as a code owner May 17, 2024 16:32
@niklasad1 niklasad1 merged commit dd343be into master May 23, 2024
13 checks passed
@niklasad1 niklasad1 deleted the jsdw-check-metadata-hash branch May 23, 2024 09:54
@FlorianFranzen
Copy link

So this break the PolkadotConfig so it only works on testchains while leaving a cryptic "Invalid Transaction: Transaction has a bad signature" on submission for all others

Barely mentioned in the change log that this could happen, e.g. even the staking miner included this release and has that issue now.

Not to mention that all 0.36 user will upgrade to this as well as soon as they run into any issues...

@niklasad1
Copy link
Member

@FlorianFranzen do you mind filing an issue in the staking-miner repo?

I wonder why it's just works on "test chains" ^^

@jsdw
Copy link
Collaborator Author

jsdw commented Jun 11, 2024

So this break the PolkadotConfig so it only works on testchains while leaving a cryptic "Invalid Transaction: Transaction has a bad signature" on submission for all others

Could you provide a way for us to reproduce this, please?

Adding the new extension to PolkadotConfig/SubstrateConfig shouldn't have broken anything; the extension is only made use of if the node's metadata is asking for it (which would have previously broken Subxt anyway, since it didn't exist), and the new extension was (and is currently being tested each night) and we haven't seen any issues around it.

@FlorianFranzen
Copy link

@jsdw I looked at in detail and tried to create a trivial example without success, so it must be particular to our use-case. The last time I looked at this was also before the runtime update on Polkasama, so that might also play a role is guiding me to believing it is the config that caused the issue.

The issue also appeared in the miner, but it seems unrelated. I currently lack the time to look into this further. Sorry for the false alert. I will post an update should I uncover anything of value.

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

Successfully merging this pull request may close these issues.

Subxt integration tests failed against latest Substrate build.
5 participants