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

client/asset/{btc,dcr}: support p2pk and fix unsupported btc utxos #1581

Merged
merged 2 commits into from
Apr 23, 2022

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Apr 12, 2022

Addressing issues revealed in #1558 (comment)

This updates the DCR and BTC packages to understand P2PK outputs.

This also fixes the BTC client packages UTXO conversion code so that
unsupported output scripts in the listunspent result are merely skipped
instead of breaking the ability to select UTXOs at all.

Comment on lines +3352 to +3427
if errors.Is(err, dex.UnsupportedScriptError) {
continue
}
return nil, nil, 0, fmt.Errorf("error reading asset info: %w", err)
}
if nfo.ScriptType == dexbtc.ScriptUnsupported || nfo.NonStandardScript {
// InputInfo sets NonStandardScript for P2SH with non-standard
// redeem scripts. Don't return these since they cannot fund
// arbitrary txns.
continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This now mirror's DCR's client code.

@chappjc chappjc added this to the 0.4.3 milestone Apr 12, 2022
@chappjc chappjc added bug bug or bugfix high priority labels Apr 12, 2022
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Maybe add p2pk tracking in TestLiveUTXO and LiveUTXOStats?

@chappjc
Copy link
Member Author

chappjc commented Apr 12, 2022

Maybe add p2pk tracking in TestLiveUTXO and LiveUTXOStats?

That was a minor rabbit hole. I ended up fixing some bugs in the btc LiveUTXOStats functions (the way it checked segwit was incorrect, maybe once before, but not now). Also got the serialized block instead of a discrete getrawtransaction for each. Lastly, added the booleanVerboseGetBlock flag for server's btc backend.

@@ -287,7 +287,7 @@ func (utxo *UTXO) Confirmations(context.Context) (int64, error) {
// See if we can find the utxo in another block.
newUtxo, err := utxo.btc.utxo(&utxo.tx.hash, utxo.vout, utxo.redeemScript)
if err != nil {
return -1, fmt.Errorf("utxo block is not mainchain")
return -1, fmt.Errorf("utxo error: %w", err)
Copy link
Member Author

@chappjc chappjc Apr 12, 2022

Choose a reason for hiding this comment

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

immature output was one of the ones left after the bug fixes. Nothing to do with mainchain anyway. We might rethink the utxo method a little because there is a maturity field so I'm not sure it should error instead of deferring to the caller to decide if immature is a problem (server is probably checking funding coins or swap txns, so it does make sense, but in terms of general operation utxo failing to provide a return for immature outputs even though it has a maturity field is strange)

I don't want to change it now though

server/asset/btc/btc.go Show resolved Hide resolved
server/asset/btc/btc.go Show resolved Hide resolved
dex/networks/dcr/script_test.go Outdated Show resolved Hide resolved
dex/networks/dcr/script_test.go Outdated Show resolved Hide resolved
dex/networks/dcr/script_test.go Outdated Show resolved Hide resolved
dex/networks/dcr/script_test.go Outdated Show resolved Hide resolved
dex/networks/dcr/script_test.go Outdated Show resolved Hide resolved
server/asset/btc/rpcclient.go Show resolved Hide resolved
client/asset/btc/btc.go Show resolved Hide resolved
server/asset/btc/rpcclient.go Outdated Show resolved Hide resolved
@chappjc chappjc force-pushed the p2pk-support branch 2 times, most recently from 66d06de to 3b93791 Compare April 21, 2022 00:35
This updates the DCR and BTC packages to understand P2PK outputs.

This also fixes the BTC client packages UTXO conversion code so that
unsupported output scripts in the listunspent result are merely skipped
instead of breaking the ability to select UTXOs at all.
server/asset/doge: add BlockDeserializer
@chappjc chappjc merged commit 105d3e7 into decred:master Apr 23, 2022
@chappjc chappjc deleted the p2pk-support branch April 23, 2022 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants