Skip to content

fix(utxo-withdraw): get hw ctx only when PrivKeyPolicy is trezor#2333

Merged
shamardy merged 2 commits intodevfrom
fix-init-withdraw-utxo
Feb 7, 2025
Merged

fix(utxo-withdraw): get hw ctx only when PrivKeyPolicy is trezor#2333
shamardy merged 2 commits intodevfrom
fix-init-withdraw-utxo

Conversation

@shamardy
Copy link
Copy Markdown
Collaborator

@shamardy shamardy commented Jan 30, 2025

This fixes task::withdraw::init for non-trezor utxo withdraws as it was failing with "error_data": "NoTrezorDeviceAvailable"

To Test:
Make sure task::withdraw::init works fine for iguana/hd wallet/trezor

@shamardy shamardy changed the title fix(utxo-withdraw): get hw ctx only when PrivKeyPolicy is trezor for utxo init_withdraw fix(utxo-withdraw): get hw ctx only when PrivKeyPolicy is trezor Jan 30, 2025
@shamardy shamardy requested a review from CharlVS January 30, 2025 14:50
Copy link
Copy Markdown

@CharlVS CharlVS left a comment

Choose a reason for hiding this comment

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

@shamardy Thank you for taking care of this so quickly. I've tested it out, and I am now getting a different error:

flutter: mm2Rpc request: {"method":"task::withdraw::init","mmrpc":"2.0","params":{"coin":"DOC","to":"RWiJB5gVjY3vRY1WWncRkUBg1vi9aRgp7P","from":{"derivation_path":"m/44'/141'/0'/0/1"},"max":true},"userpass":"45PVe8TkgwAouQKv7lqcbHqb@A1C1enC"}
flutter: RPC response: {"mmrpc":"2.0","result":{"task_id":10},"id":null}
[log] isErrorResponse: false, json: {mmrpc: 2.0, result: {task_id: 10}, id: null}
flutter: mm2Rpc request: {"method":"task::withdraw::status","mmrpc":"2.0","params":{"task_id":10,"forget_if_finished":false},"userpass":"45PVe8TkgwAouQKv7lqcbHqb@A1C1enC"}
flutter: [ERROR]: 31 12:21:27, coins::utxo::utxo_withdraw:251] INFO Trying to withdraw MAX DOC from RX5H99JPtgjXktyfQXtAgvDbe6kRiTtsAu to RWiJB5gVjY3vRY1WWncRkUBg1vi9aRgp7P

flutter: RPC response: {"mmrpc":"2.0","result":{"status":"InProgress","details":"GeneratingTransaction"},"id":null}
[log] isErrorResponse: false, json: {mmrpc: 2.0, result: {status: InProgress, details: GeneratingTransaction}, id: null}
flutter: mm2Rpc request: {"method":"task::withdraw::status","mmrpc":"2.0","params":{"task_id":10,"forget_if_finished":false},"userpass":"45PVe8TkgwAouQKv7lqcbHqb@A1C1enC"}
flutter: RPC response: {"mmrpc":"2.0","result":{"status":"Error","details":{"error":"Internal error: P2PKH script 'OP_DUP\nOP_HASH160\nOP_PUSHBYTES_20 0xeb1a21101bcf4b4ca2f727ad77dbca311d3e1cb6\nOP_EQUALVERIFY\nOP_CHECKSIG\n' built from input key pair doesn't match expected prev script 'OP_DUP\nOP_HASH160\nOP_PUSHBYTES_20 0xef12006fbbecdf69e58016de36ea3db01386e561\nOP_EQUALVERIFY\nOP_CHECKSIG\n'","error_path":"utxo_withdraw.lib.with_key_pair","error_trace":"utxo_withdraw:326] lib:150] with_key_pair:114]","error_type":"InternalError","error_data":"P2PKH script 'OP_DUP\nOP_HASH160\nOP_PUSHBYTES_20 0xeb1a21101bcf4b4ca2f727ad77dbca311d3e1cb6\nOP_EQUALVERIFY\nOP_CHECKSIG\n' built from input key pair doesn't match expected prev script 'OP_DUP\nOP_HASH160\nOP_PUSHBYTES_20 0xef12006fbbecdf69e58016de36ea3db01386e561\nOP_EQUALVERIFY\nOP_CHECKSIG\n'"}},"id":null}
[log] isErrorResponse: true, json: {mmrpc: 2.0, result: {status: Error, details: {error: Internal error: P2PKH script 'OP_DUP
      OP_HASH160
      OP_PUSHBYTES_20 0xeb1a21101bcf4b4ca2f727ad77dbca311d3e1cb6
      OP_EQUALVERIFY
      OP_CHECKSIG
      ' built from input key pair doesn't match expected prev script 'OP_DUP
      OP_HASH160

@mariocynicys
Copy link
Copy Markdown
Collaborator

mariocynicys commented Jan 31, 2025

all the utxos in the tx u wanna sign must be owned by the same p2pkh address and that address must be the activated one in the HD wallet. was that the case for the test u did? @CharlVS
oops, we actually should use the keypair of the from derivation path and not sign the tx using the activated address.

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 3, 2025

@shamardy Thank you for taking care of this so quickly. I've tested it out, and I am now getting a different error:

@CharlVS please test after the latest commit.

oops, we actually should use the keypair of the from derivation path and not sign the tx using the activated address.

@mariocynicys The last commit should fix this issue.

Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@CharlVS CharlVS left a comment

Choose a reason for hiding this comment

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

Thank you, @shamardy. I've tested it and confirmed it works as expected when withdrawing between HD wallet addresses for DOC. However, I can't confirm that it works as expected for all possible parameters as QA typically does.

CharlVS added a commit to GLEECBTC/komodo-defi-sdk-flutter that referenced this pull request Feb 4, 2025
Update KDF to `95b8f03` to fix a bug affecting HD withdrawals for UTXO coins.

KDF PR:
fix(utxo-withdraw): get hw ctx only when PrivKeyPolicy is trezor #2333: GLEECBTC/komodo-defi-framework#2333
@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 4, 2025

However, I can't confirm that it works as expected for all possible parameters as QA typically does.

Will wait for @smk762 review for this. We need to make sure that trezor works as before as well.

Copy link
Copy Markdown

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

changes LGTM

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 7, 2025

Will merge this and @KomodoPlatform/qa can test this in dev.

@shamardy shamardy merged commit 104c626 into dev Feb 7, 2025
@shamardy shamardy deleted the fix-init-withdraw-utxo branch February 7, 2025 09:07
shamardy added a commit that referenced this pull request Feb 7, 2025
…2333)

This fixes `task::withdraw::init` for non-trezor utxo withdraws as it was failing with "error_data": "NoTrezorDeviceAvailable". It also allows the user to withdraw from any address in the HD wallet using `task::withdraw::init` instead of only the activated address.
dimxy pushed a commit that referenced this pull request Feb 11, 2025
* dev:
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261)
  feat(tendermint): unstaking/undelegation (#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333)
dimxy pushed a commit that referenced this pull request Feb 16, 2025
* dev:
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261)
  feat(tendermint): unstaking/undelegation (#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333)
  feat(event-streaming): API-driven subscription management (#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (#2279)
  fix(ARRR): store unconfirmed change output (#2276)
  feat(tendermint): staking/delegation (#2322)
  chore(deps): `timed-map` migration (#2247)
  fix(mem-leak): `running_swap` never shrinks (#2301)
  chore(dep-bump): libp2p (#2326)
  refactor(build script): rewrite the main build script (#2319)
dimxy pushed a commit that referenced this pull request Feb 16, 2025
* dev:
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261)
  feat(tendermint): unstaking/undelegation (#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333)
  feat(event-streaming): API-driven subscription management (#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (#2279)
  fix(ARRR): store unconfirmed change output (#2276)
  feat(tendermint): staking/delegation (#2322)
  chore(deps): `timed-map` migration (#2247)
  fix(mem-leak): `running_swap` never shrinks (#2301)
  chore(dep-bump): libp2p (#2326)
  refactor(build script): rewrite the main build script (#2319)
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Feb 24, 2025
* dev: (24 commits)
  fix(eth-tpu): remove state from funding validation (GLEECBTC#2334)
  improvement(rpc-server): rpc server dynamic port allocation (GLEECBTC#2342)
  fix(tests): fix or ignore unstable tests (GLEECBTC#2365)
  fix(fs): make `filter_files_by_extension` return only files (GLEECBTC#2364)
  fix(derive_key_from_path): check length of current_key_material (GLEECBTC#2356)
  chore(release): bump mm2 version to 2.4.0-beta (GLEECBTC#2346)
  fix(tests): add additional testnet sepolia nodes to test code (GLEECBTC#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (GLEECBTC#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (GLEECBTC#2354)
  feat(tpu-v2): provide swap protocol versioning (GLEECBTC#2324)
  feat(wallet): add change mnemonic password rpc (GLEECBTC#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (GLEECBTC#2261)
  feat(tendermint): unstaking/undelegation (GLEECBTC#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (GLEECBTC#2333)
  feat(event-streaming): API-driven subscription management (GLEECBTC#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279)
  fix(ARRR): store unconfirmed change output (GLEECBTC#2276)
  feat(tendermint): staking/delegation (GLEECBTC#2322)
  chore(deps): `timed-map` migration (GLEECBTC#2247)
  fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants