wojake / DKM

Decentralized Key Management NodeJS library for HP dApps. Do not use in production!
8 stars 2 forks source link

ValidationError: Serialized transaction does not match original txJSON. See error.data #5

Closed jellicoe closed 1 year ago

jellicoe commented 1 year ago

Hi,

just testing the test contract and I get this ValidationError when it goes to sign the SignerListSet tx

appears to be issue in xrpl.js checkTxSerialization

possibly because

"SignerQuorum: 0" ???

so suspect comms issue between the 5 HP nodes ? where should I look first?

ncc: Version 0.34.0 ncc: Compiling file index.js into CJS 125kB dist/build/Release/secp256k1.node 2388kB dist/index.js 2513kB [6110ms] - ncc 0.34.0 command: deploy (cluster: default) sub-command: stop hpdevkit_default_node_3 hpdevkit_default_node_1 hpdevkit_default_node_4 hpdevkit_default_node_2 hpdevkit_default_node_5 sub-command: sync Applying hp.cfg overrides Applying hp.cfg overrides Applying hp.cfg overrides Applying hp.cfg overrides Applying hp.cfg overrides sub-command: start hpdevkit_default_node_4 hpdevkit_default_node_5 hpdevkit_default_node_3 hpdevkit_default_node_1 hpdevkit_default_node_2 Streaming logs of node 1: sub-command: logs 20230329 02:31:46.518 [inf][hpc] Consensus processor started. 20230329 02:31:46.518 [inf][hpc] Read request thread pool manager started. 20230329 02:31:46.520 [inf][hpc] Started listening for peer connections on 22861 20230329 02:31:46.520 [inf][hpc] Started peer managing thread. 20230329 02:31:46.521 [inf][hpc] Started listening for user connections on 8081 20230329 02:31:50.299 [inf][hpc] Not enough peers proposing to perform consensus. votes:1 needed:4 20230329 02:31:50.482 [inf][hpc] Reported violation '1' from 172.21.0.5 20230329 02:31:50.563 [inf][hpc] Reported violation '1' from 172.21.0.2 20230329 02:31:50.583 [inf][hpc] Reported violation '1' from 172.21.0.3 [HPWS.C PID+0000002D] GOTO client_closed, ERROR: client closed connection [HPWS.C PID+0000002B] GOTO client_closed, ERROR: client closed connection 20230329 02:31:59.070 [inf][hpc] Ledger created (lcl:1-75970e8f state:51b63697 patch:6a2aa25b) ValidationError: Serialized transaction does not match original txJSON. See error.data at Wallet.checkTxSerialization (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:59692:27) at Wallet.sign (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:59609:14) at SetSignerList (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:13352:50) at async Object.SetupAccount (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:13435:2) at async mycontract (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:70771:9) at async invokeCallback (/hpdevkit_vol/node1/contract_fs/mnt/rw/state/index.js:13806:9) { data: { decoded: { TransactionType: 'SignerListSet', Flags: 0, Sequence: 36539544, LastLedgerSequence: 36542882, SignerQuorum: 0, Fee: '12', Account: 'rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6', SignerEntries: [Array], Memos: [Array] }, tx: { TransactionType: 'SignerListSet', Account: 'rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6', SignerEntries: [Array], SignerQuorum: NaN, Memos: [Array], Flags: 0, Sequence: 36539544, Fee: '12', LastLedgerSequence: 36542882 } } }

wojake commented 1 year ago

It's not a comms issue between your 5 nodes since a node would only submit a SetSignerList tx in Ledger 1 (setup) when it has collected a list of all the nodes' signer address.

    const cluster_signer_keys = await NPLResponse({
        content: JSON.stringify({
            roundName: `node-key-setup-${dApp_account_address}`,
            data: node_address
        }),
        desired_count: ctx.unl.count(),
        ctx: ctx,
        timeout: GetNPLTimeoutValue(type="signerlist_setup"),
        strict: true
        // strict *must* be true, because if this node does not get *all* the nodes' signer address, it may submit a SignerListSet tx that isn't full
    });

As you can see here, strict is set to true and desired_count is set to the node's UNL node count. So, without the node being able to collect all of its UNL peers' signer address, it would return an empty list resulting in the node not participating in the account setup phase.

With that out of the way, I've never actually come across this error during setup. Would you mind sharing your code?

wojake commented 1 year ago

If the node wasn't able to collect all the nodes' signer address in time, the node's logs would look like this: SetSignerList(): This node failed to receive all ${ctx.unl.count()} nodes' keys, it will not partake in setting up the dApp's XRPL Account @ ${account_wallet.classicAddress}...

wojake commented 1 year ago

Ok never mind, it seems like your node instance did collect all of its peers' signer address since you passed the signer list length check but this may be happening due to the fact that you didn't setup dist/DKM/setup/bootstrap_state.json's values properly:

    "config": {
        "account_seed": "seed",
        "quorum": {
            "signer_quorom": 0.8,
            "voting_quorum": 0.8
        },
        "NPL_round_timeout": {
            "signerlist_setup": 6000,
            "signing": 5000,
            "key_checkup": 8000,
            "key_checkup_validation": 8000
        }
    }
jellicoe commented 1 year ago

OK found the BUG in the test bootstrap_state file (swap o for u)

look above spelt in code as signer_quorum vs signer_quorom in bootstrap

thats why SignerQuorum: NaN in my original post

you must have this hard coded somewhere if you haven't come across this issue

and we are in action now have changed bootstrap to signer_quorum

https://testnet.xrpl.org/accounts/rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6/transactions

another bug though

it tried to disable the master key before the signer list was set - but this failed - check second oldest transaction at address above

I shut down nodes and re-ran it then it worked 2nd time

see log from first failed attempt

`DKM INF: SetSignerList(): dApp's XRPL account is now controlled by its HotPocket nodes, the master key has been disabled DKM INF: SetupAccount(): dApp's XRPL account is now setup @ rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6 DKM WRN: GetSignerList(): dApp's account rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6 does not have a SignerList 20230329 08:04:38.444 [inf][hpc] Ledger created (lcl:2-9e39fc2a state:f3cec5dd patch:20ead0da) DKM WRN: GetSignerList(): dApp's account rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6 does not have a SignerList DKM: XRPL Account rBtHqo6MhAogLHsuwwybcQSjfcrUGnEaA6 has received 1 new transaction(s) !!!

hope this helps

cheers

wojake commented 1 year ago

The 2nd AccountSet attempting to disable the master key won't work since there aren't any other keys that may control the account. This happened since no node was successful enough to collect all of its peers' signer address, resulting in the cluster moving to Ledger ~2.

What we could do is, before moving to another ledger, we could perform a check on the account's state (lsfDisableMaster & SignerList object) before proceeding.

Thank you mate, you've been helpful ❤️ . Making changes until I fix a few other bugs.

shortthefomo commented 1 year ago

good check

wojake commented 1 year ago

Fixed w/ v0.2.0