Speed up BTC signing

[andrewkozlik]

Summary

This PR should reduce the singing time of a large CoinJoin transaction from 220 seconds to 44 seconds. All time measurements were made on a physical device using trezorlib to provide the transaction. The changes are outlined below:

Added a large CoinJoin transaction to the device tests

Added a CoinJoin transaction with 400 inputs and 1200 outputs of which 10 and 30, respectively, are internal. This is the largest transaction that Wasabi will produce under default settings.

Added no_serialize option to SignTx

• Implemented in TT only.
• Reduces the signing time by about one third.
• What about T1? I don't see a strong reason to implement it in T1. We want it to save time in signing extra-large transactions like CoinJoins, where time is of the essence. T1 doesn't even implement external inputs, much the less CoinJoins. Is it OK to ignore the option in T1 or should it return an error if the option is set to True? @matejcik
• Naming. I'd prefer serialize, but setting [default=true] in the protobuf definition doesn't make the protobuf parser automatically assign true, when the option is not set. So I wonder, do we always want the default to be false in protobuf? Is this a bug in the parser? @matejcik

Verify ownership proofs before transaction approval in BTC signing

This allows us to completely skip Step 3 in CoinJoin signing. Reduces the signing time by about 7 % more, because we don't have to stream the external inputs again.

Use C implementation of Bech32 decode

Reduces the signing time by almost 40 % more. I can't explain why this is. I tried to optimize the Python implementation, but it didn't help and I couldn't locate any particular operation that is responsible for the huge time difference. Maybe someone with hardware expertise can do some profiling to figure out what's going on.

Improve Bitcoin signing progress display

I decided to split the progress wheel into two phases:

• Phase 1: Loading transaction. Steps 1 and 2 of the SignTx process, i.e. everything that happens before final user confirmation. Previously we didn't show any progress for Phase 1.
• Phase 2: Signing transaction. This consists of the remaining steps of the SignTx process: prevtx verification, verification of presigned external inputs, the actual signing operations and transaction serialization.

During phase 1 we discover important information about the transaction: taproot_only, number of SegWit inputs and external inputs. This is critical for giving a good estimate of the number of operations in Phase 2.

For example, in a typical CoinJoin, Phase 1 takes about 95% of the time. Phase 2 is fast, but we wouldn't be able to tell without knowing that the transaction is Taproot-only and that most inputs are external. So if we relied only on the total number of inputs and outputs as sources of information for the time estimate, then we would grossly overestimate the overall time.

On the other hand, in a non-Taproot transaction, Phase 2 may take 95 % of the time, because the previous transactions need to be verified. The user is unlikely to even notice the "Loading transaction" progress dialog, unless the number of inputs is large.

Furthermore, differentiating between loading and signing may be useful in interactive signing to indicate to the user that no more confirmations are expected from them in the "Signing transaction" phase.

In debug mode the firmware will now check that the progress wheel completed exactly the predicted number of steps. If not, then it will return a FirmwareError. The only scenario that I know of, which won't be predicted correctly, is transaction replacement with mixed legacy + SegWit input script types. This can be fixed easily (add segwit_count to OriginalTxInfo), but I think it's not worth the extra code, because Suite doesn't allow this scenario.

Other improvements I tried

I tried reverting 2cedc68 to enable the BIP32 cache. This decreased the processing time by 22ms per internal input or output, which improves the overall time by about one second. (Not included in this PR.)

Review

@matejcik please review overall.
@onvej-sl please look at these two commits:

• d54cae0 I suggest to double-check for buffer overflow and out of bounds read.
• de951d2 this changes the BTC signing flow slightly, so let's have two reviewers check this commit for security issues.

[andrewkozlik]

In 87bc82b I tried changing no_serialize to serialize and the default to true. This change breaks device tests as well as unit tests. There seems to be something very wrong with the handling of default values in our code base.

If I don't specify serialize in SignTx, then pytest ... -vv log shows this:

[2022-09-19 18:42:28,612] trezorlib.client DEBUG: sending message: SignTx
SignTx (13 bytes) {
    coin_name: 'Testnet',
    inputs_count: 2,
    outputs_count: 1,
}

Trezor log shows this:

58574977910 trezor.wire DEBUG received message contents:
SignTx {
    inputs_count: 2
    lock_time: 0
    coin_name: 'Testnet'
    version: 1
    amount_unit: 0
    outputs_count: 1
    decred_staking_ticket: False
    timestamp: None
    branch_id: None
    version_group_id: None
    expiry: None
    serialize: False
}

This would indicate two bugs, neither of which I can explain:

• It's as if the __init__() method of SignTx in trezorlib here isn't getting called, so True is never assigned to the serialized parameter on the trezorlib side.
• At the same time on the Trezor side the serialized parameter is assigned False instead of True as the default.

[matejcik]

• It's as if the __init__() method of SignTx in trezorlib here isn't getting called, so True is never assigned to the serialized parameter on the trezorlib side.

It's not, and that is by design. (though very confusing)
The generated code is better thought of as a fakery for type-checking purposes than actual code; when we drop python 3.6 compatibility, the generated constructors will fly out the window.

• At the same time on the Trezor side the serialized parameter is assigned False instead of True as the default.

Indeed, there seems to be a bug in pb2py where it expects the value to be a capital-T True. I'd need to check but I guess that shoud either be lowercase, or maybe a case-insensitive match?

[matejcik]

It's not, and that is by design. (though very confusing)

except that it does not matter and the functionality should be there: #2527

[andrewkozlik]

Indeed, there seems to be a bug in pb2py where it expects the value to be a capital-T True. I'd need to check but I guess that shoud either be lowercase, or maybe a case-insensitive match?

I think it should be lowercase. The change seems to work. Waiting for the CI to complete. BTW, I had to run make clean then make build_unix for the fix to take effect, which indicates a missing dependency.

[andrewkozlik]

It seems to me that we can easily avoid streaming the inputs that have an ownership proof here. See this draft. Does it make sense?

The reason why I chose to use the has_presigned boolean value over a set of presigned indices is because it seemed like a premature optimization to use a set. It would optimize the signing speed only when all of the following conditions are satisfied:

• There is at least one presigned input.
• There are many external inputs with an ownership proof.
• All internal inputs are taproot.

So far we don't have a use-case for that. But it would be useful for CoinJoins where some participants don't have the ability to produce an ownership proof for their input and produce a presigned input. I am not against the optimization. I guess using a set vs. bool here won't make a relevant difference in terms of RAM.

[andrewkozlik]

We can add this commit to the PR: 2700aa2

[andrewkozlik]

@onvej-sl?

I added the commit to this PR as 0c24201.

[grdddj]

Just a small review/some ideas, I cannot judge the signing details very much

[mmilata]

Changes to signing flow look good as far as my limited understanding of it goes. Stared a bit at the C changes and did not find any issues. Good job with the speedup!

[matejcik]

Is it OK to ignore the option in T1

Yes. This doesn't break compatibility in any way either, the caller is free to ignore the serialized chunk.

Naming. I'd prefer serialize

this is resolved now, right?

[matejcik]

I tried to optimize the Python implementation, but it didn't help and I couldn't locate any particular operation that is responsible for the huge time difference.

could it be the checksum calculation? native ints are only like 28 bits IIRC, so anything bigger is a mpz bigint

[andrewkozlik]

I tried to optimize the Python implementation, but it didn't help and I couldn't locate any particular operation that is responsible for the huge time difference.

could it be the checksum calculation? native ints are only like 28 bits IIRC, so anything bigger is a mpz bigint

I suppose it could be a part of the problem. When I was researching the speedup, I couldn't localize the time-demanding part to any particular step inside bech32.decode().