test: MiniWallet: fix fee calculation for P2PK and check tx vsize #22089

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):

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

Thanks for the explanation in the PR. I like your approach, but I'm wondering if this will still be accurate about create_self_transfer: """Create and return a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed.""" Because right now, using a fee rate of Decimal("0.003") the P2PK tx will have a fee of Decimal('0.0005025'), but the exact fee rates should either be Decimal('0.000504') for 168 vbytes, or Decimal('0.000501') for 167 vbytes

So it seems like the fee rate won't be exact anymore unless i'm missing something. I feel like it might be a problem for some tests if create_self_transfer created transactions where the fee rate wasn't exactly the same as what was specified.

It's probably not an issue when no fee rate has been specified, but perhaps in the case where the user does specify a fee rate then the signature could be restricted to 71 bytes?

@DariusParvin: Thanks for reviewing, you brought up an interesting point here.

Indeed, the promise "Fee may be exact or at most one satoshi higher than needed." does currently not apply for MiniWallet transaction created in P2PK mode (and on the master branch it's obviously totally off, since the vsize was not set properly). As far as I can tell the only way to achieve this would be with a constant signature size, otherwise we have a chicken and egg situation: for creating a signature we need to have the output amount already set (which determines the fee), but for a achieving a certain fee-rate we need to know the transaction size which is not known until the signature is created.

So it seems like the fee rate won't be exact anymore unless i'm missing something. I feel like it might be a problem for some tests if create_self_transfer created transactions where the fee rate wasn't exactly the same as what was specified.

Good question, I don't know enough of the "big picture" of all tests to know whether having exact fees is something that is really needed (and if it really is, if the anyone-can-spend modes are not sufficient then?).

I could generally think of two possible solutions:

• Keep the current logic in the PR and only adapt the comment (e.g. something like "For anyone-can-spend outputs, the fee may be exact or at least most one satoshi higher than needed. For P2PK outputs, the fee is approximated due to varying signature size.")
• Grind signatures to always have fixed size (potentially slow since .sign_ecdsa has to be executed about twice on average to get a single signature), as you suggested:

It's probably not an issue when no fee rate has been specified, but perhaps in the case where the user does specify a fee rate then the signature could be restricted to 71 bytes?

Since the fee_rate is a parameter with default value, I don't think we can tell whether the user specified an explicit fee rate or not (at least not with the current interface of create_self_transfer()). What I could think of though is introducing a parameter to sign_tx that specifies if the signature should be forced to have a fixed size (false by default), and setting it to true in the create_self_transfer() method. That maybe is a good compromise: for transaction created with the wallet, the fee is calculated exactly, for external uses of .sign_tx the signature size and fee don't matter, since the transaction probably doesn't end up in the mempool anyway, but only in blocks.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

@theStack

What I could think of though is introducing a parameter to sign_tx that specifies if the signature should be forced to have a fixed size (false by default), and setting it to true in the create_self_transfer() method. That maybe is a good compromise: for transaction created with the wallet, the fee is calculated exactly, for external uses of .sign_tx the signature size and fee don't matter, since the transaction probably doesn't end up in the mempool anyway, but only in blocks.

I think that's a reasonable compromise. The only thing is I think it might be better to have the default be True instead, mainly because the fee rate is quite important for a few tests (e.g. getting included in a block, replaced in the mempool, fee limits) and it could be very hard to debug if the fee rate is random between test runs.

Rebased on master, changed sign_tx to create fixed-size signatures (71 bytes) by default (as proposed by DariusParvin), force-pushed and updated the PR description.

@theStack

What I could think of though is introducing a parameter to sign_tx that specifies if the signature should be forced to have a fixed size (false by default), and setting it to true in the create_self_transfer() method. That maybe is a good compromise: for transaction created with the wallet, the fee is calculated exactly, for external uses of .sign_tx the signature size and fee don't matter, since the transaction probably doesn't end up in the mempool anyway, but only in blocks.

I think that's a reasonable compromise. The only thing is I think it might be better to have the default be True instead, mainly because the fee rate is quite important for a few tests (e.g. getting included in a block, replaced in the mempool, fee limits) and it could be very hard to debug if the fee rate is random between test runs. @DariusParvin: After repeatedly executing the test feature_csv_activation.py (the only one so far making extensive use of sign_tx()), I didn't see significant differences in execution time, so I guess grinding to a fixed size is okay. I agree that in a test framework it is preferrable to have reproducible results rather than squeezing out a little less execution time. If that is a concern in future test that create lots of signatures, the parameter fixed_length can be set to False explicitely.

Concept ACK d6d2ab984547be4a9f7ba859a2a4c9ac9bfbf206