fix(p2pk): validate expected pubkey correctly for p2pk inputs by mariocynicys · Pull Request #2408 · GLEECBTC/komodo-defi-framework

mariocynicys · 2025-04-07T11:17:25Z

For p2pk inputs, the pubkey isn't included in the scriptSig. The scriptSig only includes the signature and that's it.

This means we can't just read the pubkey from the scriptSig and deduce that such pubkey is the owner of the utxo (and then compare it with the expected pubkey, taker or maker). But rather we need to verify the signature in the scriptSig using the expected pubkey.

ref. https://learnmeabitcoin.com/technical/script/p2pk/
Fixes #2339

and integrate it in check_all_utxo_inputs_signed_by_pub

mm2src/coins/utxo/utxo_common.rs

specifically, don't require coin. this makes it easier to test also add a fixme regarding pubkey size in the scriptpub, it might be: - OP_PUSHBYTES_33 33-byte-compressed-pubkey OP_CHECKSIG - OP_PUSHBYTES_65 65-byte-full-pubkey OP_CHECKSIG

and change the parameters of the func for now to allow for uncompressed pubkeys since i can't find any compressed pubkey transactions online that i can use for testing

also add a fixme for a bug found with this impl for overwintered transactions: those transactions need to have the amount field set, which generally isn't set because it is fetched from UtxoTx object which doesn't have the amount set as well.

found in scriptPub. i.e. we will assume the public key in scriptpub was compressed and try to verify the signature, if we fail, we will assume it's uncompressed and try to verify the signature again. if either of these two operations succeed, verification succeeds, if both fail, verification fails.

mariocynicys · 2025-04-09T07:31:37Z

force pushed to move out unrelated commits away to #2410

mariocynicys · 2025-04-15T11:05:44Z

mm2src/coins/utxo/utxo_common.rs

+                // FIXME: For overwintered txs, we also need to set the input amount as it's used in sighash calcuations!
+                unsigned_tx.consensus_branch_id = coin.as_ref().conf.consensus_branch_id;


to fix this, we need to query for the amount of this input. so one rpc request per p2pk input.

i think the efficient solution for this is to disable this check all together for p2pk spends on overwintered txs.

I think you may just query the previous scriptPubKey for p2pk inputs.
This would be a one for all solution as you would not need to check the signature but just retrieve the pubkey from the previous scriptPubKey

that's the same communication overhead, but i think is a neater solution.

we wouldn't wanna perform it though if the transaction/coin isn't overwintered, because we already have all the info needed to verify the taker is the owner of the utxos without the need for a network request.

we wouldn't wanna perform it though if the transaction/coin isn't overwintered, because we already have all the info needed to verify the taker is the owner of the utxos without the need for a network request.

I prefer to not use a network request when it's not needed.

borngraced

great work! I have some notes. Generally, I noticed you don't really use line breaks. Please do

mm2src/mm2_bitcoin/keys/src/public.rs

mm2src/coins/utxo/utxo_common.rs

borngraced · 2025-04-16T10:45:25Z

mm2src/coins/utxo/utxo_common.rs

+                let mut unsigned_tx: TransactionInputSigner = tx.clone().into();
+                // FIXME: For overwintered txs, we also need to set the input amount as it's used in sighash calcuations!
+                unsigned_tx.consensus_branch_id = coin.as_ref().conf.consensus_branch_id;


Would it also be okay to modify the original tx's consensus_branch_id

this field doesn't get set when we get the TransactionInputSigner from a plain Transaction (check https://github.com/KomodoPlatform/komodo-defi-framework/blob/a97afb467eaf24fe6530f3f7341d7173a3777c41/mm2src/mm2_bitcoin/script/src/sign.rs#L177).

it must be this one of the coin anyways and matches with the one used by the taker. otherwise all the transaction signing will break and not verify.

i will might remove such mutation anyway as consensus_branch_id is only used with overwintered txs, and we already have a problem with them with them: namely, the amount must be set for signing to produce correct signature.

if we can we should for sure.

mm2src/coins/utxo/utxo_common.rs

rust-analyzer's auto import did that ;p

mm2src/coins/utxo/utxo_common.rs

dimxy · 2025-04-17T12:38:42Z

mm2src/coins/utxo/utxo_common.rs

+            &pubkey_script,
+            // FIXME: But P2PK scripts never use segwit as signature version. Should we hardcode this to SignatureVersion::Base or ::ForkId?
+            //        UPD: I think we can safely remove this fixme if the following reasoning holds:
+            //        Since we are using p2pk here, this means the coin isn't segwit. right? but it's the coin supplied from the caller anyways


I think, KDF does not mix segwit and non segwit inputs but another app could.
(here we actually check txns created by a remote app)

upon digging down, looks like we list p2pk utxos for segwit address:
https://github.com/KomodoPlatform/komodo-defi-framework/blob/b161cd44aff8ea5f140018c2589ee90d00952454/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs#L805-L809

we should check address.address_format.is_segwit() and not add p2pk outputs if so. i.e. we should associate p2pk outputs only with coins enabled in non-segwit mode, i.e. coins enabled for p2pkh outputs.

another alternative is to change signature_version passed to signature_hash_to_sign for each input based on its utxo type (p.s. actually this will look inconsistent; raising the question why is a coin labeled as segwit but it can spend other address types)

we should check address.address_format.is_segwit() and not add p2pk outputs

maybe just do not allow to add a pubkey to segwit addr

715f504 done here

mm2src/coins/utxo/utxo_common.rs

shamardy · 2025-05-20T22:29:03Z

@mariocynicys please merge with latest dev for wasm tests job to pass.

…e expected pubkey this simplifies things a lot as no verification is now needed. the down side is that we now query the rpc for each and every p2pk input and not just overwintered ones. this is prolly OK tho since p2pk inputs aren't that popular anyways. ofc we assume the rpc server isn't lying

mariocynicys · 2025-05-21T09:07:24Z

@dimxy @shamardy

the latest commit applies this suggestion #2408 (comment)

the one before applies the amount fetching + sig verification suggestion.

im fine with either one of them (or removing the check for overwintered p2pk all together also).

mm2src/coins/utxo/utxo_common.rs

…ainst the expected pubkey" This reverts commit 8fdd7d3. we will stick with only performing the network request when needed (so only for overwintered)

as there is no need for the verbose transaction

mm2src/mm2_bitcoin/keys/src/public.rs

mm2src/coins/utxo/utxo_common.rs

…irst

…2pk verification

that's either a coinbase tx or an invalid tx with no inputs (most probably the latter). if it's invalid, we don't really care about that within the scope of this function

since it won't be needed most of the time as it's only used when an input is p2pk

onur-ozkan

LGTM, thanks!

… empty" This reverts commit a2c8f8f.

* dev: (29 commits) fix(p2pk): validate expected pubkey correctly for p2pk inputs (#2408) chore(docs): update old urls referencing atomicdex or old docs pages (#2428) improvement(p2p): remove hardcoded seeds (#2439) fix(evm-api): find enabled erc20 token using platform ticker (#2445) chore(docs): add DeepWiki badge to README (#2463) chore(core): organize deps using workspace.dependencies (#2449) feat(db-arch): more dbdir to address_dir replacements (#2398) chore(build-artifacts): remove duplicated mm2 build artifacts (#2448) feat(pubkey-banning): expirable bans (#2455) fix(eth-balance-events): serialize eth address using AddrToString (#2440) chore(deps): remove base58 and replace it completely with bs58 (#2427) feat(tron): initial groundwork for full TRON integration (#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (#2316) deps(timed-map): bump to 1.3.1 (#2413) improvement(tendermint): safer IBC channel handler (#2298) chore(release): complete v2.4.0-beta changelogs (#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (#2431) improvement(watchers): re-write use-watchers handling (#2430) fix(evm): make withdraw_nft work in HD mode (#2424) feat(taproot): support parsing taproot output address types ...

* lr-swap-wip: (37 commits) fix custom token error name fix getting chain_id from protocol_data refactor (review): use dedicated large error cfg, add new fn to FromApiValueError, fix TODO, use experimental namespace for lr rpc, more Ticker alias feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459) fix(test): fix HD Wallet message signing tests (GLEECBTC#2474) improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464) feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432) fix(p2pk): validate expected pubkey correctly for p2pk inputs (GLEECBTC#2408) chore(docs): update old urls referencing atomicdex or old docs pages (GLEECBTC#2428) improvement(p2p): remove hardcoded seeds (GLEECBTC#2439) fix(evm-api): find enabled erc20 token using platform ticker (GLEECBTC#2445) chore(docs): add DeepWiki badge to README (GLEECBTC#2463) chore(core): organize deps using workspace.dependencies (GLEECBTC#2449) feat(db-arch): more dbdir to address_dir replacements (GLEECBTC#2398) chore(build-artifacts): remove duplicated mm2 build artifacts (GLEECBTC#2448) feat(pubkey-banning): expirable bans (GLEECBTC#2455) fix(eth-balance-events): serialize eth address using AddrToString (GLEECBTC#2440) chore(deps): remove base58 and replace it completely with bs58 (GLEECBTC#2427) feat(tron): initial groundwork for full TRON integration (GLEECBTC#2425) ...

add a p2pk signature verifier

4e61580

and integrate it in check_all_utxo_inputs_signed_by_pub

mariocynicys added status: in progress priority: urgent Critical tasks requiring immediate action. bug: swap labels Apr 7, 2025

mariocynicys commented Apr 7, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Show resolved Hide resolved

mariocynicys added status: pending review and removed status: in progress labels Apr 9, 2025

mariocynicys added 7 commits April 9, 2025 09:29

add test for p2pk detection function

d638bc9

add a test for verify_p2pk_input_pubkey

6f24451

and change the parameters of the func for now to allow for uncompressed pubkeys since i can't find any compressed pubkey transactions online that i can use for testing

add more details to the signature version fixme

1f0e0f3

fix compilation error

d127ade

add to_secp256k1_pubkey() to Public

2ce3c97

mariocynicys force-pushed the p2pk-spends-in-swaps branch from 95d86db to d1b2e4e Compare April 9, 2025 07:29

simplify does_script_spend_p2pk

a97afb4

shamardy requested a review from dimxy April 14, 2025 23:03

shamardy added the 2.5.0-beta label Apr 14, 2025

mariocynicys commented Apr 15, 2025

View reviewed changes

borngraced suggested changes Apr 16, 2025

View reviewed changes

mariocynicys added 2 commits April 16, 2025 14:35

review(sami): resolve a couple of comments - readability, DRY, cleanness

8a9923a

import the correct Deref

ba82af5

rust-analyzer's auto import did that ;p

dimxy reviewed Apr 17, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved

review(dimxy): fix typo in 'signature'

b262954

dimxy reviewed Apr 17, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Show resolved Hide resolved

borngraced mentioned this pull request Apr 17, 2025

fix(p2pk): allow listing and spending p2pk utxos created with uncompressed pubkeys #2410

Draft

dimxy reviewed Apr 17, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Show resolved Hide resolved

dimxy reviewed Apr 17, 2025

View reviewed changes

mariocynicys commented May 20, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Show resolved Hide resolved

mariocynicys added 2 commits May 20, 2025 12:41

fix utxo inputs sig check doc comment

bdb3d05

be more idomatic with matches

f435492

mariocynicys dismissed shamardy’s stale review via f435492 May 20, 2025 10:42

mariocynicys added 4 commits May 21, 2025 09:10

Merge remote-tracking branch 'origin/dev' into p2pk-spends-in-swaps

f33ff36

get the amount from rpc for overwintered txs

04dfbf4

add a test for overwintered txs p2pk input verification

75eabfa

shamardy reviewed May 23, 2025

View reviewed changes

mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved

mariocynicys added 2 commits May 23, 2025 10:00

Revert "review(dimxy): just query for the prev_output and check it ag…

86b0949

…ainst the expected pubkey" This reverts commit 8fdd7d3. we will stick with only performing the network request when needed (so only for overwintered)

use get_transaction_bytes instead of get_versbose_transaction

95411c0

as there is no need for the verbose transaction

onur-ozkan suggested changes May 27, 2025

View reviewed changes

mariocynicys added 6 commits May 27, 2025 08:52

review(onur): inline a small frequently used method

72d6754

review(onur): reorder if conditions for readability - make the p2pk f…

cffe6c0

…irst

review(onur): move scriptsig utitlity method to impl Script

440f1e7

review(onur): separate prev tx fetching and amount setting logic in p…

0a84be1

…2pk verification

review(onur): add a pre-check to exit early if the inputs are empty

a2c8f8f

that's either a coinbase tx or an invalid tx with no inputs (most probably the latter). if it's invalid, we don't really care about that within the scope of this function

lazily calculate unsigned_tx only when needed

77aa1b1

since it won't be needed most of the time as it's only used when an input is p2pk

onur-ozkan previously approved these changes May 27, 2025

View reviewed changes

Revert "review(onur): add a pre-check to exit early if the inputs are…

e3d1edb

… empty" This reverts commit a2c8f8f.

mariocynicys dismissed onur-ozkan’s stale review via e3d1edb May 27, 2025 09:02

shamardy approved these changes May 28, 2025

View reviewed changes

shamardy merged commit 82144f6 into dev May 28, 2025
24 checks passed

shamardy deleted the p2pk-spends-in-swaps branch May 28, 2025 08:51

shamardy mentioned this pull request May 28, 2025

P2PK input cause taker fee validation to fail #2339

Closed

shamardy mentioned this pull request Jun 16, 2025

chore(release): v2.5.0-beta #2491

Merged

		// FIXME: For overwintered txs, we also need to set the input amount as it's used in sighash calcuations!
		unsigned_tx.consensus_branch_id = coin.as_ref().conf.consensus_branch_id;

Conversation

mariocynicys commented Apr 7, 2025

Uh oh!

Uh oh!

mariocynicys commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented May 20, 2025

Uh oh!

mariocynicys commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

borngraced left a comment •

edited

Loading