Skip to content

merge #12196, #13697: Add scantxoutset RPC method #4187

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

Merged
merged 3 commits into from
Jul 26, 2021

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Jun 4, 2021

Overview

This pull request presupposes the merger of #4186 into develop. Only the last two commits are meant to be reviewed. This pull request introduces the scantxoutset RPC command without segregated witness-specific logic. To actually compare psbt with scantxout, go here

Motivation

During initial preparation for bitcoin#18242 ("Add BIP324 encrypted p2p transport de-/serializer" by jonasschnelli) and the backporting of bitcoin@78c312c, a difference in class and function names were noticed and that in turn lead to a series of pull requests the lead us to this one.

The motivation is to reduce future merge conflicts when backporting pull requests from Bitcoin Core.

Things To Do

* Fix test/descriptor_tests.cpp / possibly readapt them for Dash

test/descriptor_tests.cpp:53: error: in "descriptor_tests/descriptor_test": check parse_priv has failed
test/descriptor_tests.cpp:57: error: in "descriptor_tests/descriptor_test": check keys_priv.keys.size() has failed
unknown location:0: fatal error: in "descriptor_tests/descriptor_test": memory access violation at address: 0x00000000: no mapping at fault address
test/descriptor_tests.cpp:58: last checkpoint

Contents

bitcoin#12196, bitcoin#13697

Disclosures

  • This is a work in progress and needs external review. Dash-specific changes have not been tested, only compilation has been ensured. Running the client on a testnet may be necessary.
  • Tests fail. (see "Things To Do") Only Python ones do but at least we know how to get around it, see comment below

@PastaPastaPasta PastaPastaPasta marked this pull request as draft June 8, 2021 17:39
@kwvg
Copy link
Collaborator Author

kwvg commented Jun 9, 2021

This took more time than it should've. Had a better post with Markdown, reference URLs and more but an ill-timed Cmd+Q and it was gone, here's the abridged version.

Test failure due to memory access error sounds scary, thought it was due to errors while porting but after porting it again, same problems, chucking it in a debugger and turns out the test vector private key was a nullptr (which is used very liberally whenever an error is found, for some reason), since nullptr is used too liberally in PSBT logic so needed to set some breakpoints and step through to find the exact cause. Turns out it was mismatched prefixes.

Yes. Prefix mismatch stole five hours of my time. Welp, at least I know how to use Xcode's visual debugger and finally moved on from gdb, it's lldb season at Darwinland.

Don't know how to generate new tests so here is how I adapted the existing test vectors to ones that should work with Dash, should be reproducible.

#
# Step by step generation of
# modified test vectors for scantxout
#
# All values represented unless stated otherwise are in hex
# All transformations and hashing should be done in HEX mode, not in TEXT mode
#

# Raw random private key         (just generated so I know its de facto length)
__b2c3e210e52c74b36ab0d9203e8e7cb9442c2281998edd46a744d133d2e800b9__________

# Known-good Bitcoin private key (taken from scantxout tests)
# Base58 representation: L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1
80e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855011fa4de16

# Known-good Dash private key    (generated within Dash Qt using getaddress and dumpprivkey)
ccf58c277bb348b0ab476b16c1635bc8eb711daf5cc7965aae11d020f6fdfde8f701a9429834

# Format                         (as understood from https://en.bitcoin.it/wiki/Wallet_import_format)
**f58c277bb348b0ab476b16c1635bc8eb711daf5cc7965aae11d020f6fdfde8f7__--------
** - prefix | __ - isCompressed | -------- - checksum

# Bitcoin's prefix is '80', Dash has 'cc' (convert '204' to hex, see chainparams or known good Dash Private Key)
cc65f08e5800944dcbeaf956b58bb08c571674fdd7440277492ac205ac935fde0501130abb47

# Known good Bitcoin private key with checksum, compress bit and prefix removed
# Notice it's the same length as the raw random private key
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

# Add Dash prefix and isCompressed suffix
cce3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b85501

# First SHA256 iteration
a7f53fcec9d1378865831eb3f022ecdd6806dfa82473bc03a1701bacb295c78f

# Second SHA256 iteration
b41738f51164e711c444441c5050368b0d1ceb2ef138f23e08e37df0ad52e29b

# Checksum
b41738f5

# Append it to key
# Final Base58 representation: XJvEUEcFWCHCyruc8ZX5exPZaGe4UR7gC5FHrhwPnQGDs1uWCsT2
cce3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b85501b41738f5

# importprivkey XJvEUEcFWCHCyruc8ZX5exPZaGe4UR7gC5FHrhwPnQGDs1uWCsT2 works
# on Dash Core, so suffice to say, this is a valid private key. We can repeat the
# steps above for the uncompressed privkey (5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)
# except we omit the isCompressed bit (again, omit, not set to zero, that bit needs to be gone)
# and eventually we'll get 7sH936MDoVPFVk2VoaCM5yW8P3BfPyffnZECyaHrZwfLgWpS13e, which
# is the uncompressed private key that we use here

@kwvg
Copy link
Collaborator Author

kwvg commented Jul 9, 2021

Rebased and updated, Python tests pending (inherited from PSBT)

@kwvg kwvg marked this pull request as ready for review July 13, 2021 09:21
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Jul 13, 2021
@kwvg
Copy link
Collaborator Author

kwvg commented Jul 14, 2021

Rebased on top of develop, post PSBT merge!

@kwvg kwvg force-pushed the scantxout branch 2 times, most recently from 0000300 to 4aaad46 Compare July 14, 2021 14:09
@PastaPastaPasta PastaPastaPasta added this to the 18 milestone Jul 14, 2021
@kwvg kwvg force-pushed the scantxout branch 2 times, most recently from f67c001 to 4104118 Compare July 15, 2021 14:14
@kwvg
Copy link
Collaborator Author

kwvg commented Jul 20, 2021

# L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1
**f58c277bb348b0ab476b16c1635bc8eb711daf5cc7965aae11d020f6fdfde8f7__--------

SHA256d: b41738f51164e711c444441c5050368b0d1ceb2ef138f23e08e37df0ad52e29b
Final: cce3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b85501b41738f5
Generated: XJvEUEcFWCHCyruc8ZX5exPZaGe4UR7gC5FHrhwPnQGDs1uWCsT2

# 5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss
**f58c277bb348b0ab476b16c1635bc8eb711daf5cc7965aae11d020f6fdfde8f7__--------

SHA256d: caf1acd1bebe5fac7e7e0d816c2dcbafa23fee3adc72ba513c741ef40f429b5f
Final: cce3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855caf1acd1
Generated: 7sH936MDoVPFVk2VoaCM5yW8P3BfPyffnZECyaHrZwfLgWpS13e

Running 380 test cases...
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]
test/descriptor_tests.cpp:91: error: in "descriptor_tests/descriptor_test": check spks.size() == ref.size() has failed [2 != 4]

@UdjinM6, combo breaks a couple things. The branch with changes are here

@UdjinM6
Copy link

UdjinM6 commented Jul 20, 2021

pls see 7dacf7a

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

@UdjinM6 UdjinM6 requested a review from PastaPastaPasta July 21, 2021 22:51
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

@PastaPastaPasta PastaPastaPasta merged commit edf0552 into dashpay:develop Jul 26, 2021
@kwvg kwvg deleted the scantxout branch July 18, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants