Skip to content

Conversation

theStack
Copy link
Contributor

This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, as suggested by MarcoFalke. Using that mode in the test feature_csv_activation.py, all txs submitted to the mempool follow the standard policy, i.e. -acceptnonstdtxn=1 can be removed.

Possible follow-ups:

  • Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
  • Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
  • Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)

@DrahtBot DrahtBot added the Tests label May 13, 2021
@maflcko
Copy link
Member

maflcko commented May 17, 2021

Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)

I think that test also uses the mempool, which doesn't like raw scripts. I guess an alternative to that is to wrap all scripts into a hashed output first, which is cumbersome :(. Or ignore specific reject reasons, when available.

@bitcoin bitcoin deleted a comment from Modusto25 May 17, 2021
@michaelfolkson
Copy link

Concept ACK. A clear improvement to be able to generate P2PK outputs in the functional tests in addition to anyone-can-spend outputs.

@theStack
Copy link
Contributor Author

Thanks for the reviews! #20753 looks interesting, though at first glance it seems to be discussed controversially. Will look at it more in detail within the next days. Another example where the MiniWallet with P2PK support helps is feature_dersig.py, which used the wallet for creating a DER-signature (i.e. ignoring specific reject reasons doesn't help in this case).

@theStack theStack force-pushed the 202005-test-miniwallet-support-p2pk branch from 6c0a1b7 to 7c44bc7 Compare May 17, 2021 18:05
@theStack
Copy link
Contributor Author

theStack commented May 17, 2021

Force-pushed with the following changes:

  • changed MiniWallet.sign_tx() method to not return the tx again, as the signing works in-place by directly modifying the first input's scriptSig (thanks to @reemuru whose review comment inspired me to do this!)
  • changed mode selection structure to be more friendlier w.r.t. changing to an enum (thanks to @MarcoFalke for suggesting). The mode selection looks now like the following:
raw_script use_p2pk Mode
True X raw OP_TRUE output
X True raw P2PK ([pubkey] OP_CHECKSIG) output
False False bech32 P2WSH address, containing hash of OP_TRUE (default)

Using the MiniWallet in P2PK mode, all transactions submitted to the
mempool are following the standard policy now, i.e. the node command
line parameter '-acceptnonstdtxn=1' is not needed anymore.
@theStack theStack force-pushed the 202005-test-miniwallet-support-p2pk branch from 7c44bc7 to 4bea301 Compare May 17, 2021 18:13
@laanwj
Copy link
Member

laanwj commented May 18, 2021

Code review ACK 4bea301

@maflcko maflcko merged commit 778b920 into bitcoin:master May 24, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 25, 2021
4bea301 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64 test: MiniWallet: add P2PK support (Sebastian Falbesoner)

Pull request description:

  This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](bitcoin#21900 (comment)). Using that  mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.

  Possible follow-ups:
  * Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
  * Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
  * Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)

ACKs for top commit:
  laanwj:
    Code review ACK 4bea301

Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request May 25, 2021
…r output mode

6cebac5 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to #21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support).
  Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment bitcoin/bitcoin#21945 (comment)), a new Enum type `MiniWalletMode` is introduced that can hold the following values:

  - ADDRESS_OP_TRUE
  - RAW_OP_TRUE
  - RAW_P2PK

  Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose).

ACKs for top commit:
  MarcoFalke:
    cr ACK 6cebac5

Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request May 25, 2021
…sig.py

3e05a57 test: use MiniWallet (P2PK mode) for feature_dersig.py (Sebastian Falbesoner)

Pull request description:

  This PR enables one more of the non-wallet functional tests (feature_dersig.py) to be run even with the Bitcoin Core wallet disabled. A valid DER-signature is created by using the recently introduced P2PK-Mode of the MiniWallet (bitcoin#21945).

ACKs for top commit:
  MarcoFalke:
    cr ACK 3e05a57

Tree-SHA512: 0fb8da8ed8b47f68bcb57301eb4f0171a6c9e44539b7554626969347e5d6f80b3b9085f2cc160cd038a990f0d81b8b614846260fbed43b5f950d77f1b7aa81cf
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 25, 2021
… mode

6cebac5 test: MiniWallet: introduce enum type for output mode (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to bitcoin#21945 which lifted the number of MiniWallet's tx output modes from 2 to 3 (by adding P2PK Support).
  Since the current way of specifying the mode on the ctor via two booleans is ugly and error-prone (see table in comment bitcoin#21945 (comment)), a new Enum type `MiniWalletMode` is introduced that can hold the following values:

  - ADDRESS_OP_TRUE
  - RAW_OP_TRUE
  - RAW_P2PK

  Also adds documentation that should guide the user on which mode is useful for what etc. with a summary table. (Can also be split up in a separate commit or shortened if that is desired, maybe it's considered to be too verbose).

ACKs for top commit:
  MarcoFalke:
    cr ACK 6cebac5

Tree-SHA512: cbbc10806d9d9e62829548094415e9f1a281cd247b9a9fc7f7f33b923c723aa03e7bc3024623a77fb1f7da4d73455fa8244840f746980d32acdad97ee12100da
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2021
…sig.py

3e05a57 test: use MiniWallet (P2PK mode) for feature_dersig.py (Sebastian Falbesoner)

Pull request description:

  This PR enables one more of the non-wallet functional tests (feature_dersig.py) to be run even with the Bitcoin Core wallet disabled. A valid DER-signature is created by using the recently introduced P2PK-Mode of the MiniWallet (bitcoin#21945).

ACKs for top commit:
  MarcoFalke:
    cr ACK 3e05a57

Tree-SHA512: 0fb8da8ed8b47f68bcb57301eb4f0171a6c9e44539b7554626969347e5d6f80b3b9085f2cc160cd038a990f0d81b8b614846260fbed43b5f950d77f1b7aa81cf
maflcko pushed a commit that referenced this pull request Jun 21, 2021
…k tx vsize

d6d2ab9 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)

Pull request description:

  This PR is a follow-up to #21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.

  Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):

  | DER signature size [bytes]  | #occurences (ratio) |
  | ------------- | ------------- |
  | 71  | 498893 (49.89%) |
  | 70 | 497244 (49.72%) |
  | 69 | 3837 (0.38%) |
  | 68 | 22 (0.0022%) |

  Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially.     Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).

  ~~The idea of grinding signatures to a fixed size (similar to #13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~

  For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816

ACKs for top commit:
  MarcoFalke:
    Concept ACK d6d2ab9

Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 24, 2021
…nd check tx vsize

d6d2ab9 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner)
ce024b1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner)

Pull request description:

  This PR is a follow-up to bitcoin#21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch.

  Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey):

  | DER signature size [bytes]  | #occurences (ratio) |
  | ------------- | ------------- |
  | 71  | 498893 (49.89%) |
  | 70 | 497244 (49.72%) |
  | 69 | 3837 (0.38%) |
  | 68 | 22 (0.0022%) |

  Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially.     Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed).

  ~~The idea of grinding signatures to a fixed size (similar to bitcoin#13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~

  For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816

ACKs for top commit:
  MarcoFalke:
    Concept ACK d6d2ab9

Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32
@theStack theStack deleted the 202005-test-miniwallet-support-p2pk branch July 31, 2021 20:07
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants