MinaProtocol / mina

Mina is a cryptocurrency protocol with a constant size blockchain, improving scaling while maintaining decentralization and security.
https://minaprotocol.com
Apache License 2.0
1.99k stars 525 forks source link

The hash result calculated by MinaSigner's hashPayment is inconsistent with the hash displayed on the chain. #12681

Open zhangtory opened 1 year ago

zhangtory commented 1 year ago

Preliminary Checks

Description

I use Mina-Signer to sign and calculate hash by hashPayment,but the txHash returned by sdk is different from the txhash showed on explorer.

It seems that the method of calculating the hash is inconsistent.

Can this bug be fixed? Or is it caused by my improper use?

https://github.com/MinaProtocol/mina/issues/12328 Don't know if that's the reason stated here. Whether I use 2.0.0 or 1.7.0, the result is wrong.

Steps to Reproduce

env: Node.js v18.12.1

package.json: "dependencies": { "mina-signer": "2.0.0" }

code:

const Client = require("mina-signer");
const sdk = new Client({ network: "mainnet" });

let keys = {
    privateKey: "secret",
    publicKey: "B62qksJ2giaMucoA4dEJDqpqfsMsz3s1o8MMvQdjCnMPwHDvwe7gFyj"
};
let self = {
    privateKey: 'secret',
    publicKey: 'B62qn6duzMSZRq4Y6Yth7q2g4iN9kEXpTR5aNFD6jE9AWMBthmrQeUG'
}

const signedPayment = sdk.signPayment({
    to: self.publicKey,
    from: keys.publicKey,
    fee: "1000000",
    nonce: "5",
    memo: "",
    amount: "1000000",
}, keys.privateKey);
console.log(signedPayment);
console.log(sdk.hashPayment(signedPayment, { berkeley: true }));
console.log(sdk.hashPayment(signedPayment, { berkeley: false }));

log:

{
  signature: '7mXU7zeJfj6yzoLSHRezMqDv6pY1gmcwQkr5tSwKEiLxxS9C9LDfxSFM1qsQ67uh1bo51RFJfNHsCFzJTMGo48B3mKhwF4XB',
  publicKey: 'B62qksJ2giaMucoA4dEJDqpqfsMsz3s1o8MMvQdjCnMPwHDvwe7gFyj',
  data: {
    to: 'B62qn6duzMSZRq4Y6Yth7q2g4iN9kEXpTR5aNFD6jE9AWMBthmrQeUG',
    from: 'B62qksJ2giaMucoA4dEJDqpqfsMsz3s1o8MMvQdjCnMPwHDvwe7gFyj',
    fee: '1000000',
    amount: '1000000',
    nonce: '5',
    memo: '',
    validUntil: '4294967295'
  }
}
CkpYhYnxMtSBeq9vpphLYqzijeqQc9ooEmLycdVs1nExJyMnNXQk2
CkpZAVTZuz1UHygS1v9CU7g9sQ5BkZvPAciSMubZJgNruPQvcuTK1

Expected Result

view on minaexplorer: CkpaFqsnCSDVugMJDNVwG22LNnaCH2hmJDi238cN1TYKdfah8Lf1G

Actual Result

sdk result: CkpYhYnxMtSBeq9vpphLYqzijeqQc9ooEmLycdVs1nExJyMnNXQk2 or CkpZAVTZuz1UHygS1v9CU7g9sQ5BkZvPAciSMubZJgNruPQvcuTK1

How frequently do you see this issue?

Always

What is the impact of this issue on your ability to run a node?

Low

Status

no need.

Additional information

No response

zhangtory commented 1 year ago

@mitschabaude pls take a look, thx.

mitschabaude commented 1 year ago

@garethtdavies do you know how minaexplorer (on mainnet) calculates the tx hash? Is it just using the result returned from the graphql endpoint?

garethtdavies commented 1 year ago

@mitschabaude yes what is returned via GraphQL. I thought is was going to be this but that seems to have been closed https://github.com/MinaProtocol/mina/issues/12328. The initial discussion on that was here https://discord.com/channels/484437221055922177/1047309312428617869/1047954052346024077

mitschabaude commented 1 year ago

Prematurely closed I guess :D

psteckler commented 1 year ago

The discrepancy might have something to do with the version tag used in the OCaml Base58 code for Transaction_hash. Those are removed in develop, but they're still there in the mainnet code. Does the signer include that?

mitschabaude commented 1 year ago

I'm pretty sure that the version tag change came into the code after publishing 2.0.0. Also, it doesn't work with an earlier version either. So I assume something else must have changed

psteckler commented 1 year ago

In develop, the version tags were removed in #11400 from June, 2022.

psteckler commented 1 year ago

In release/2.0.0-mina-signer, the code from #11400 is there. ppx_version is in src/lib/ppx_version. No version tags in serialized data (except where they're added by @@@with_all_version_tags or @@@with_top_version_tag).

But Transaction_hash does use a top-tag, exactly for compatibility with mainnet.

So it looks like something else changed.

psteckler commented 1 year ago

The "something else" is, I believe, that in the signer code, the signatures in payments and delegations are replaced with a dummy signature before hashing. That was done so that hashes could be calculated from information in the archive db, which does not include signatures.

In the mainnet code, hashing is done with the signatures intact.

psteckler commented 1 year ago

Although hash_signed_command_v1 does use the version tags; it's a digest of the Base58Check of the command (including the intact signature). So I'm not sure what the bug is.