feat(trezor): add segwit support for withdraw with trezor #1984

dimxy · 2023-10-04T11:39:41Z

Add support for witness inputs and outputs for withdraw tx to sign it with trezor device
Add test tool to sign withdraw tx with witness inputs/outputs with trezor and emulator
Closes #1685

shamardy

Great work! Only minor comments!
I reviewed the main code changes, will check the ported emulator code and the tests in the next and last iteration :)

mm2src/coins/utxo/utxo_withdraw.rs

mm2src/coins/utxo_signer/src/sign_params.rs

mm2src/trezor/src/utxo/unsigned_tx.rs

mm2src/coins/utxo/utxo_tests.rs

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

shamardy · 2023-10-04T18:27:19Z

mm2src/trezor/src/transport/udp.rs

@@ -0,0 +1,160 @@
+/// This source file was borrowed from Trezor repo and modified to integrate into this project
+/// Adds udp transport to interact with trezor emulator
+/// To build emulator use this repo: https://github.com/trezor/trezor-firmware, build with build-docker.sh for the desired branch (tag)


It might be a good idea in the future to build and run the trezor emulator in CI to have trezor dockerized tests, please add this to the checklist here #964 (comment) or create a new checklist in a new comment on the issue.

Maybe just create an issue?
(I am just not sure how to find that checklist as it appears buried inside another issue)

Maybe just create an issue?

Sure, please create an issue.

shamardy · 2023-10-04T18:30:27Z

mm2src/trezor/src/transport/udp.rs

@@ -0,0 +1,160 @@
+/// This source file was borrowed from Trezor repo and modified to integrate into this project


For ported code, this guidelines #1861 (review) #1861 (comment) are generally a good idea so we can track the changes in the commit history. If it's not easy to do that now, no problem but please remember this for the future.

I tried to change commits to add the origin ported file to the history

Thanks, before merging you should squash other commits except the ported code commit, then I should merge to dev without squashing so we would have 2 commits only.

Could you please clarify, 2 or 3 commits?
in the guidelines above it is suggested to have 3 commits: initial ported code, changes to the ported code and the rest of the work

Yeah, 3 commits is the best way :)
I thought 2 is not a big difference since the other commit will have the changes too, but 3 is better to have a seperate commit related to changes to ported the code only.

shamardy

Thanks you for the fixes! Next review iteration where it's all also minor stuff to make the code better :)

mm2src/crypto/src/hw_client.rs

mm2src/trezor/src/transport/udp.rs

mm2src/mm2_main/tests/integration_tests_common/mod.rs

mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs

shamardy

LGTM!
All unresolved comments are non-blockers, so you can work on them while testing for this feature is underway.

shamardy · 2023-10-18T18:00:42Z

@dimxy I don't think we need to document anything for @KomodoPlatform/qa since this works the same as it does for non-segwit UTXO coins. The only thing needs mentioning is that we need to change the segwit coins derivation path, e.g. here https://github.com/KomodoPlatform/coins/blob/1194a05e76c416437c33658b8eacdf235216d23a/coins#L2074 since the purpose for segwit is 84, so this should be m/84'/0'. I think this applies to all coins not only btc but need your confirmation @dimxy.

dimxy · 2023-10-19T13:01:37Z

@dimxy I don't think we need to document anything for @KomodoPlatform/qa since this works the same as it does for non-segwit UTXO coins. The only thing needs mentioning is that we need to change the segwit coins derivation path, e.g. here https://github.com/KomodoPlatform/coins/blob/1194a05e76c416437c33658b8eacdf235216d23a/coins#L2074 since the purpose for segwit is 84, so this should be m/84'/0'. I think this applies to all coins not only btc but need your confirmation @dimxy.

I agree that we should fix the purpose field for segwit coins to make it correct according to the BIP84 standard.
We should also validate it to match another field in the coin config: #1995.
By fixing the purpose we won't harm user funds as the GUI currently uses the HD API only for the trezor wallet, for which segwit coins are not enabled yet

dimxy · 2023-11-02T10:02:43Z

rebased on the latest dev

smk762 · 2023-11-03T10:48:08Z

Can you please include some working method examples? I'm only getting errors here at the moment.

./version.sh

{
    "result": "1.1.0-beta_3909721",
    "datetime": "2023-11-02T15:43:41+05:00"
}

./init_trezor_withdraw_A.sh DGB-segwit dgb1q6drqvl3u8sukfsu4lm3qsk28jr3fahja302wyy 4.321 1

{
    "mmrpc": "2.0",
    "result": {
        "task_id": 4
    },
    "id": null
}

./init_trezor_withdraw_status.sh 4

{
    "mmrpc": "2.0",
    "result": {
        "status": "Error",
        "details": {
            "error": "Unexpected 'from' address: Unknown BIP32 error: decoding error",
            "error_path": "utxo_common",
            "error_trace": "utxo_common:3068] utxo_common:3067]",
            "error_type": "UnexpectedFromAddress",
            "error_data": "Unknown BIP32 error: decoding error"
        }
    },
    "id": null
}

./task_withdraw_init_A.sh DGB-segwit dgb1q6drqvl3u8sukfsu4lm3qsk28jr3fahja302wyy 4.321 "m/84'/20'/0'/0/1"

{
    "mmrpc": "2.0",
    "result": {
        "task_id": 5
    },
    "id": null
}

./task_withdraw_status.sh 5

{
    "mmrpc": "2.0",
    "result": {
        "status": "Error",
        "details": {
            "error": "Internal error: Unexpected interaction request: PinMatrix3x3",
            "error_path": "utxo_withdraw.with_trezor.response",
            "error_trace": "utxo_withdraw:346] with_trezor:31] response:66]",
            "error_type": "InternalError",
            "error_data": "Unexpected interaction request: PinMatrix3x3"
        }
    },
    "id": null
}

dimxy · 2023-11-03T13:35:19Z

I have a couple of samples in the code. They are marked 'ignored' as require trezor connected or emulator running:

cargo test -p mm2_main  -- --nocapture --ignored test_withdraw_from_trezor_p2pkh_rpc
cargo test -p mm2_main  -- --nocapture --ignored test_withdraw_from_trezor_segwit_rpc

they both create and print a signed transaction on BTC testnet. the path used is m/44'/1'/0'/0/0 and m/84'/1'/0'/0/0, you'll need some value on that account.
(you could use the emulator, then add --features trezor-udp option)

dimxy · 2023-11-03T13:39:42Z

"Internal error: Unexpected interaction request: PinMatrix3x3",

I think pin is not supported yet (I tried my samples with no pin set on trezor)
Update: seems pin is supported, I received a pin request during task::init_trezor::init rpc call. I used your init_trezor_useraction_pin.sh to respond and it worked fine. After that I managed to create a withdraw successfully.
But after some time another withdraw ended with the same error "Unexpected interaction request: PinMatrix3x3". So it looks like pin is supported at the first init call only...

"error": "Unexpected 'from' address: Unknown BIP32 error: decoding error"

this apparently relates to improper derivation path in the withdraw params

dimxy · 2023-11-06T10:32:31Z

added a small fix for the trezor withdraw test (removed an unused object)

onur-ozkan

Nice work!

Some change notes from my side:

mm2src/coins/utxo_signer/src/sign_params.rs

mm2src/coins_activation/src/lib.rs

mm2src/coins_activation/src/utxo_activation/mod.rs

onur-ozkan · 2023-11-07T14:58:06Z

mm2src/mm2_main/Cargo.toml

@@ -20,6 +20,7 @@ run-docker-tests = []
 # TODO
 enable-solana = []
 default = []
+trezor-udp = ["crypto/trezor-udp"] # use for tests to connect to trezor emulator over udp


use for tests to connect to trezor emulator over udp

If we enable this by default here, wouldn't it be enabled for release binaries as well?

@dimxy Is this fixed? I see that it's not enabled by default now.

Sorry, I missed this.
I tried to mark the code as #[cfg(all(test, feature = "trezor-udp"))] but in this case the udp support is not included when I run tests using this feature. I guess, this is because the tests are in a different crate.

onur-ozkan · 2023-11-07T15:00:02Z

mm2src/crypto/src/hw_client.rs

+                #[cfg(feature = "trezor-udp")]
+                // try also to connect to emulator over UDP
+                if let Some(trezor) = try_to_connect::<trezor::transport::udp::UdpAvailableDevice>().await? {
+                    return Ok(trezor);
+                }


I guess it should be enough to check whether if mm2 running on debug mode or not? Adding a feature for just this piece of code seems an overkill to me.

My idea for adding udp as a feature was that normally we would use those tests for trezor with a physical device (that is why I marked them 'ignored' so you need to have a device to run them) and only who wishes to build and use emulator would enable this feature.
(Later we will add ci tests which include emulator auto build and start)

/// Tool to run withdraw directly with trezor device or emulator (no rpc version, added for easier debugging)
/// run cargo test with '--ignored' option

But we have many other tests marked as ignored too, so this test will end up with lots of other tests running and most of them probably will fail. Here we should actually use the feature flag e.g., cargo test --features enable-device-tests.

Okay, thank you for this idea, I'll add this feature for device test

I see that run-device-tests feature is added, please confirm that this is fixed @dimxy

Yes I added a feature "run-device-tests" (to invoke trezor tests, if needed)

ca333 · 2023-11-10T11:03:08Z

mm2src/trezor/Cargo.toml

@@ -22,10 +22,14 @@ serde_derive = "1.0"

 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
 bip32 = { version = "0.2.2", default-features = false, features = ["alloc", "secp256k1-ffi"] }
+async-std = { version = "1.5" }


secure code reviewed async-std

…s, fixed doc comments

… emulator support (via udp), add test config helpers, add layer to connect to Trezor or emulator over usb and udp

…me, better naming and constraints, thanks to shamardy)

…ared for extended use

shamardy

🔥

* dev: (24 commits) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984) chore(config): remove vscode launchjson (KomodoPlatform#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (KomodoPlatform#2015) feat(UTXO): balance event streaming for Electrum clients (KomodoPlatform#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (KomodoPlatform#1930) fix(p2p): handle encode_and_sign errors (KomodoPlatform#2038) chore(release): add changelog entries for v2.0.0-beta (KomodoPlatform#2037) chore(network): write network information to stdout (KomodoPlatform#2034) fix(price_endpoints): add cached url (KomodoPlatform#2032) deps(network): sync with upstream yamux (KomodoPlatform#2030) fix(config): accept a string as rpcport value (KomodoPlatform#2026) feat(nft): move db lock, add tx fee and confirmations (KomodoPlatform#1989) chore(network): update seednodes for netid 8762 (KomodoPlatform#2024) chore(network): add todo on peer storage behaviour (KomodoPlatform#2025) chore(network): exclude `168.119.236.249` from the seednode list (KomodoPlatform#2021) feat(network): deprecate 7777 network (KomodoPlatform#2020) chore(release): bump mm2 version to 2.0.0-beta (KomodoPlatform#2018) feat(UTXO swaps): kmd burn plan impl (KomodoPlatform#2006) chore(docs): fix the link to simple market maker in README.md (KomodoPlatform#2011) ...

* evm-hd-wallet: (27 commits) Fix todo comments Fix HDAddressOps::Address trait bounds fix(indexeddb): fix IDB cursor.continue_() call after drop (KomodoPlatform#2028) security bump for `h2` (KomodoPlatform#2062) fix(makerbot): allow more than one prices url in makerbot (KomodoPlatform#2027) fix(wasm worker env): refactor direct usage of `window` (KomodoPlatform#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (KomodoPlatform#2039) refactor(utxo): refactor utxo output script creation (KomodoPlatform#1960) feat(ETH): balance event streaming for ETH (KomodoPlatform#2041) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984) chore(config): remove vscode launchjson (KomodoPlatform#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (KomodoPlatform#2015) feat(UTXO): balance event streaming for Electrum clients (KomodoPlatform#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (KomodoPlatform#1930) fix(p2p): handle encode_and_sign errors (KomodoPlatform#2038) chore(release): add changelog entries for v2.0.0-beta (KomodoPlatform#2037) chore(network): write network information to stdout (KomodoPlatform#2034) fix(price_endpoints): add cached url (KomodoPlatform#2032) deps(network): sync with upstream yamux (KomodoPlatform#2030) ...

* dev: feat(zcoin): ARRR WASM implementation (KomodoPlatform#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (KomodoPlatform#2046) fix(indexeddb): set stop on success cursor condition (KomodoPlatform#2067) feat(config): add `max_concurrent_connections` to mm2 config (KomodoPlatform#2063) feat(stats_swaps): add gui/mm_version in stats db (KomodoPlatform#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (KomodoPlatform#2028) security bump for `h2` (KomodoPlatform#2062) fix(makerbot): allow more than one prices url in makerbot (KomodoPlatform#2027) fix(wasm worker env): refactor direct usage of `window` (KomodoPlatform#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (KomodoPlatform#2039) refactor(utxo): refactor utxo output script creation (KomodoPlatform#1960) feat(ETH): balance event streaming for ETH (KomodoPlatform#2041) chore(release): bump mm2 version to 2.1.0-beta (KomodoPlatform#2044) feat(trezor): add segwit support for withdraw with trezor (KomodoPlatform#1984)

dimxy added the in progress Changes will be made from the author label Oct 4, 2023

dimxy self-assigned this Oct 4, 2023

shamardy self-requested a review October 4, 2023 16:17

shamardy reviewed Oct 4, 2023

View reviewed changes

dimxy force-pushed the trezor-segwit branch from 9310b78 to d1a6a53 Compare October 10, 2023 17:26

shamardy added under review and removed in progress Changes will be made from the author labels Oct 13, 2023

shamardy reviewed Oct 13, 2023

View reviewed changes

dimxy force-pushed the trezor-segwit branch from 14cc94b to 0f45e36 Compare October 17, 2023 19:20

shamardy previously approved these changes Oct 18, 2023

View reviewed changes

shamardy added 1.1.0-beta sec-review no-deps QA docs labels Oct 18, 2023

dimxy dismissed shamardy’s stale review via 5d194c8 October 19, 2023 06:52

smk762 mentioned this pull request Oct 30, 2023

update segwit derivation paths KomodoPlatform/coins#851

Open

dimxy force-pushed the trezor-segwit branch from 5d194c8 to fb715cf Compare November 2, 2023 10:00

onur-ozkan requested changes Nov 7, 2023

View reviewed changes

ca333 reviewed Nov 10, 2023

View reviewed changes

ca333 previously approved these changes Nov 10, 2023

View reviewed changes

shamardy removed sec-review no-deps labels Nov 10, 2023

shamardy added 2.1.0-beta and removed 2.0.0-beta labels Dec 6, 2023

dimxy added 17 commits December 21, 2023 18:58

add witness input and change output support for trezor

6598acd

added trezor spending info for witness, also refactored match branche…

1bc01b0

…s, fixed doc comments

Original source for UDP transport to connect to Trezor emulator

e6bfa05

added modifications to udp.rs to integrate it

ac7df8c

add tests for withdraw from witness and p2pkh with trezor, add trezor…

6bdd6df

… emulator support (via udp), add test config helpers, add layer to connect to Trezor or emulator over usb and udp

refactor trezor ConnectableDeviceWrapper (use borrow instead of consu…

642a2a5

…me, better naming and constraints, thanks to shamardy)

add mm2 stop for trezor tests

762c3d6

fix fmt

dc0e04d

fix use mm_ctx_with_custom_db_with_conf for non wasm

6e0de54

remove unused param in test

0a82dfa

Refactor rpc task_handle and trezor request processor to make them sh…

812a200

…ared for extended use

add pin and passphrase processing for trezor sign_utxo_tx

da7361f

make as_base_shared fn more generic

d784eaa

put trezor tests in a module, add trezor pin handling in test init loop

7c0a1dc

fix wasm build for trezor init

ef10df9

make a few fn's inline

ad8a86d

refactor: eliminate unused TaskHandle aliases

bbe849f

dimxy force-pushed the trezor-segwit branch from f37a8ec to bbe849f Compare December 21, 2023 14:30

shamardy removed QA docs labels Dec 21, 2023

shamardy approved these changes Dec 21, 2023

View reviewed changes

shamardy merged commit c83778c into dev Dec 21, 2023
25 of 32 checks passed

shamardy deleted the trezor-segwit branch December 21, 2023 15:03

shamardy mentioned this pull request Jan 15, 2024

Unable to withdraw from Segwit addresses in Trezor mode #1685

Closed

shamardy mentioned this pull request Aug 1, 2024

chore(release): v2.1.0-beta #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(trezor): add segwit support for withdraw with trezor #1984

feat(trezor): add segwit support for withdraw with trezor #1984

dimxy commented Oct 4, 2023

shamardy left a comment

shamardy Oct 4, 2023

dimxy Oct 11, 2023

shamardy Oct 13, 2023

shamardy Oct 4, 2023

dimxy Oct 10, 2023

shamardy Oct 13, 2023

dimxy Oct 17, 2023

shamardy Oct 17, 2023

shamardy left a comment

shamardy left a comment

shamardy commented Oct 18, 2023 •

edited

Loading

dimxy commented Oct 19, 2023

dimxy commented Nov 2, 2023

smk762 commented Nov 3, 2023

dimxy commented Nov 3, 2023 •

edited

Loading

dimxy commented Nov 3, 2023 •

edited

Loading

dimxy commented Nov 6, 2023

onur-ozkan left a comment

onur-ozkan Nov 7, 2023

shamardy Nov 29, 2023 •

edited

Loading

dimxy Dec 14, 2023

onur-ozkan Nov 7, 2023

dimxy Nov 13, 2023

onur-ozkan Nov 13, 2023

dimxy Nov 13, 2023

shamardy Nov 29, 2023

dimxy Dec 14, 2023

ca333 Nov 10, 2023

shamardy left a comment

		@@ -0,0 +1,160 @@
		/// This source file was borrowed from Trezor repo and modified to integrate into this project

feat(trezor): add segwit support for withdraw with trezor #1984

feat(trezor): add segwit support for withdraw with trezor #1984

Conversation

dimxy commented Oct 4, 2023

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Oct 18, 2023 • edited Loading

dimxy commented Oct 19, 2023

dimxy commented Nov 2, 2023

smk762 commented Nov 3, 2023

dimxy commented Nov 3, 2023 • edited Loading

dimxy commented Nov 3, 2023 • edited Loading

dimxy commented Nov 6, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Oct 18, 2023 •

edited

Loading

dimxy commented Nov 3, 2023 •

edited

Loading

dimxy commented Nov 3, 2023 •

edited

Loading

shamardy Nov 29, 2023 •

edited

Loading