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

CNTools 10.2.3 #1636

Merged
merged 8 commits into from
Apr 30, 2023
Merged

CNTools 10.2.3 #1636

merged 8 commits into from
Apr 30, 2023

Conversation

Scitz0
Copy link
Contributor

@Scitz0 Scitz0 commented Apr 28, 2023

Description

When adding multiple HW signing keys in the same command, you need equally many output witness files.

Which issue it fixes?

closes #1634

How has this been tested?

Yes

@Scitz0 Scitz0 requested review from rdlrt and TrevorBenson April 28, 2023 07:17
@TrevorBenson
Copy link
Collaborator

@Scitz0 This is the error I get when using cntools.sh -b cntools-10.2.3 for registering a Trezor wallet:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >> WALLET >> REGISTER
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Online mode  -  The default mode to use if all keys are available

Hybrid mode  -  1) Go through steps to build a transaction file
                2) Copy built tx file to offline computer
                3) Sign it using 'Sign Tx' with keys on offline computer
                   (CNTools started in offline mode '-o' without node connection)
                4) Copy the signed tx file back to online computer and submit using 'Submit Tx'

Selected value: [o] Online

# Select wallet to register (only non-registered wallets shown)
Selected wallet: TrezorWallet-extra2 (Funds: 100.000000 Ada)


INFO: follow directions on hardware device to witness the transaction
Warning! A superfluous HW signing file specified (1 of 2), the witness was not created.
Writing to file '/tmp/cnode/cntools/tx.witness_ly3XH3Q1Me'.
Warning! A superfluous output file specified (2 of 2), the file was not written to.
Command failed: transaction sign-witness  Error: Failed to decode neither the cli's serialisation format nor the ledger's CDDL serialisation format. TextEnvelope error: /tmp/cnode/cntools/tx.witness_OnEKXZHZY9: TextEnvelope aeson decode error: Error in $: not enough input
TextEnvelopeCddl error: /tmp/cnode/cntools/tx.witness_OnEKXZHZY9: Could not JSON decode TextEnvelopeCddl file at: /tmp/cnode/cntools/tx.witness_OnEKXZHZY9 Error: Error in $: not enough input

press any key to proceed ..

@TrevorBenson
Copy link
Collaborator

TrevorBenson commented Apr 28, 2023

@Scitz0
I found the following release notes Cardano HW CLI v1.10.0 release notes:

Fixed:

  • Ledger no longer creates a staking witness for staking key registration certificate (consistently with Trezor)
  • superfluous witness output files are reported as a warning, missing witness output files are reported as an error

I can sign the tx.raw with both stake.hwsfile AND payment.hwsfile, dropping either results in a problem.
Signing with only stake.hwsfile

Warning! A superfluous HW signing file specified (1 of 1), the witness was not created.
Warning! A superfluous output file specified (1 of 1), the file was not written to.

Signing with only payment.hwsfile

Error: Missing signing file for certificate

Signing with both works, but the current CNTools 10.2.3 also tries to create 2 separate output files per --hw-signing-file which is where the failure about not being written (2 of 2 in the warnings/errors) comes from. After that the transaction assemble tries to use the missing second witness output, and that is where registration breaks down:

2023-04-28 21:22:34 UTC [ACTION] /home/guild/.local/bin/cardano-cli transaction assemble --tx-body-file /tmp/cnode/cntools/tx.raw --witness-file /tmp/cnode/cntools/tx.witness_BbqqQkA1cH --witness-file /tmp/cnode/cntools/tx.witness_QPCyPKlTNi --out-file /tmp/cnode/cntools/tx.signed_1682716954
2023-04-28 21:22:34 UTC [ERROR]  Command failed: transaction sign-witness  Error: Failed to decode neither the cli's serialisation format nor the ledger's CDDL serialisation format. TextEnvelope error: /tmp/cnode/cntools/tx.witness_QPCyPKlTNi: TextEnvelope aeson decode error: Error in $: not enough input
2023-04-28 21:22:34 UTC [ERROR]  TextEnvelopeCddl error: /tmp/cnode/cntools/tx.witness_QPCyPKlTNi: Could not JSON decode TextEnvelopeCddl file at: /tmp/cnode/cntools/tx.witness_QPCyPKlTNi Error: Error in $: not enough input

Taking the above and testing shows one less error when only 1 --out-file is used with both the stake and payment hwsfile's, but it still produces a superfluous warning. This seems unintuitive since this appears to be the correct usage.

$ $ cardano-hw-cli transaction witness --tx-file /tmp/cnode/cntools/tx.raw --hw-signing-file /opt/cardano/cnode/priv/wallet/TrezorWallet-extra2/stake.hwsfile --out-file /tmp/cnode/cntools/tx.witness_9yXrdLBKpl --testnet-magic 141 --hw-signing-file /opt/cardano/cnode/priv/wallet/TrezorWallet-extra2/payment.hwsfile 
Warning! A superfluous HW signing file specified (1 of 2), the witness was not created.
Writing to file '/tmp/cnode/cntools/tx.witness_9yXrdLBKpl'.
$ /home/guild/.local/bin/cardano-cli transaction assemble --tx-body-file /tmp/cnode/cntools/tx.raw --witness-file /tmp/cnode/cntools/tx.witness_9yXrdLBKpl --out-file /tmp/cnode/cntools/tx.signed_XXXXXXXXXX
$ cardano-cli transaction submit --tx-file /tmp/cnode/cntools/tx.signed_XXXXXXXXXX

I then tested deregistration since I submitted the registration manually, deregistration in the 10.2.3 cntools for HW wallets looks good:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >> WALLET >> DE-REGISTER
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Online mode  -  The default mode to use if all keys are available

Hybrid mode  -  1) Go through steps to build a transaction file
                2) Copy built tx file to offline computer
                3) Sign it using 'Sign Tx' with keys on offline computer
                   (CNTools started in offline mode '-o' without node connection)
                4) Copy the signed tx file back to online computer and submit using 'Submit Tx'

Selected value: [o] Online

# Select wallet to de-register (only registered wallets shown)
Selected wallet: TrezorWallet-extra2 (Funds: 97.998462 Ada)


INFO: follow directions on hardware device to witness the transaction
Writing to file '/tmp/cnode/cntools/tx.witness_hw2pfqOruh'.
Writing to file '/tmp/cnode/cntools/tx.witness_TquRBEiTIv'.
Transaction successfully submitted.

Waiting for new block to be created (timeout = 600 slots, 600s)
INFO: press any key to cancel and return (won't stop transaction)

VOS_ada successfully de-registered from chain!
Key deposit fee that will be refunded : 2.000000 Ada

press any key to proceed ..

Worked just fine. So De Registering requires both the stake.hwsfile and the payment.hwsfile and does not print ANY superfluous messages when witnessing the transaction. This seems odd because doing the same exact thing for registering warns, why not now (rhetorical question, more for the cardano-hw-cli team). It also produces two separate output files instead of one, one for each witness hwsfile. 🤷

I plan to do additional testing on the delegation, but I need to go back through and manually register the wallet again. I'll report back once I go through more multikey transactions, and keep my eyes open for a commit removing the second output file from the witness and input to the assemble.

@TrevorBenson
Copy link
Collaborator

I opened this ticket upstream w/ VacuumLabs, hopefully it helps clarify the correct steps and/or gets the warning removed entirely vacuumlabs/cardano-hw-cli#163

…r certain types like register.

Also add --change-output-key-file to tell HW device that its own key for less intrusive messages on device.
@Scitz0
Copy link
Contributor Author

Scitz0 commented Apr 29, 2023

ceb621b should fix it so that all types now successfully can be witnessed. Though the warning on registration will still show, you can ignore this for now.

@TrevorBenson
Copy link
Collaborator

ceb621b should fix it so that all types now successfully can be witnessed. Though the warning on registration will still show, you can ignore this for now.

Confirmed, this looks like a winner. I went ahead and tested both a Ledger and Trezor to confirm cardano-hw-cli produced the expected behaviors.

CNTools 10.2.3

Tested on guild (141) network

Device Trezor Model T

  • De-Registered account manually registered.
  • Registered same account.
  • Delegated account to AHL

Device Ledger Nano X

  • Registered account
  • Delegated account to AHL
  • De-Registered account

The warnings and errors are printed, but the basic transactions all work.

Copy link
Collaborator

@TrevorBenson TrevorBenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I went through online and hybrid/offline registration with hardware wallets as well as more multisig pool creation. Each test case resulted in successful transactions submitted to the chain.

@TrevorBenson
Copy link
Collaborator

TrevorBenson commented Apr 30, 2023

Leaving PR for @Scitz0 or @rdlrt to merge as the label is still set for do-not-merge.

@Scitz0 Scitz0 merged commit 31764e6 into alpha Apr 30, 2023
@Scitz0 Scitz0 deleted the cntools-10.2.3 branch April 30, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants