Trezor's RFC6979 operation diverges from that of Bitcoin core

[guidovranken]

Bitcoin recently merged a PR which changes the RFC6979 logic slightly: bitcoin-core/secp256k1#1064

This change manifested during differential fuzzing of both libraries. Here's an example input/output:

Operation:
operation name: ECDSA_Sign
ecc curve: secp256k1
private key: 22222222222222222222222222222222222222222222222222222222222222222222222222222
cleartext: {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20} (32 bytes)
nonce source: RFC 6979
digest: NULL
Module secp256k1 result:
X: 84279556694870309735507709563194569800636593243238480998490490018474863819318
Y: 103352824344139482227715594130379040953147412873655551133553784360725214630958
R: 103054904516600407832745405359194456204833007267465256495413130796542780192747
S: 42057782440274948553724453459631334779856220983958300744152873085702181448330
Module trezor-firmware result:
X: 84279556694870309735507709563194569800636593243238480998490490018474863819318
Y: 103352824344139482227715594130379040953147412873655551133553784360725214630958
R: 99970470784785202501425791218580761315684492068983010033898911979393996459919
S: 54683905977549248098326634960786247631805780693695193563622382772855979798501

cleartext denotes the input message that is passed unhashed to ecdsa_sign().

Is this something you would like to address? If not that's fine, but I need to know so I can change my fuzzer logic not to abort on RFC6979 differences between Trezor and Bitcoin.

[prusnak]

I am for changing the behaviour in our library as well to match libsecp256k1.

@onvej-sl can you please have a look what needs to be changed in our rfc6979 code to match the new behaviour?

The change in libsecp256k1 was this commit: bitcoin-core/secp256k1@45f37b6

[onvej-sl]

This branch should fix this issue. What still needs to be done is to fix tests, see Andrew's comments in #2089.

[grdddj]

This branch should fix this issue. What still needs to be done is to fix tests, see Andrew's comments in #2089.

There seem to be some issues with the indentation now, that is probably not the original issue

• https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/2132644432

Does the tests issue somehow involve the scriptPubKey check? I do not see the Andrew's commit enforcing this in that branch

[grdddj]

When I run the tests locally (after fixing the indentation), I have one failure:

>       assert (
            serialized_tx.hex()
            == "010000000001045d77b6e482d770031ad3ce3423727cc1707bc2c82e729b1189d2b60aa1a73e8c0000000017160014a33c6e24c99e108b97bc411e7e9ef31e9d5d6164ffffffff7b350e3faca092f39883d7086cdd502c82b6f0314ab61541b062733edef156790000000000ffffffff852e125137abca2dd7a42837dccfc34edc358c72eefd62978d6747d3be9315900000000000ffffffff9b117a776a9aaf70d4c3ffe89f009dcd23210a03d649ee5e38791d83902ec33a020000006b483045022100f6bd64136839b49822cf7e2050bc5c91346fc18b5cf97a945d4fd6c502f712d002207d1859e66d218f705b704f3cfca0c75410349bb1f50623f4fc2d09d5d8df0a3f012103bae960983f83e28fcb8f0e5f3dc1f1297b9f9636612fd0835b768e1b7275fb9dffffffff05a861000000000000160014d1a739f628f7eca55e8b99e7f32b22dcdbf672d4581b0000000000001976a91402e9b094fd98e2a26e805894eb78f7ff3fef199b88acf41a00000000000017a9141ff816cbeb74817050de585ceb2c772ebf71147a870000000000000000186a1674657374206f66206f705f72657475726e206461746110270000000000002251205a02573f7b39770ac53f73d161dc86f5104c6812bac297cb6ba418f6f1219c070247304402205fae7fa2b5141548593d5623ce5bd82ee18dfc751c243526039c91848efd603702200febfbe3467a68c599245ff89055514f26e146c79b58d932ced2325e6dad1b1a0121021630971f20fa349ba940a6ba3706884c41579cd760c89901374358db5dd545b90247304402201b21212100c84207697cebb852374669c382ed97cbd08afbbdfe1b302802161602206b32b2140d094cf5b7e758135961c95478c8e82fea0df30f56ccee284b79eaea012103f6b2377d52960a6094ec158cf19dcf9e33b3da4798c2302aa5806483ed4187ae01404a81e4b7f55d6d4a26923c5e2daf3cc86ed6030f83ea6e7bb16d7b81b988b34585be21a64ab45ddcc2fb9f17be2dfeff6b22cf943bc3fc8f125a7f463af428ed0000000000"
        )
E       AssertionError: assert '010000000001...ce00000000000' == '010000000001...8ed0000000000'
E         Skipping 1373 identical leading characters in diff, use -v to show
E         - 187ae01404a81e4b7f55d6d4a26923c5e2daf3cc86ed6030f83ea6e7bb16d7b81b988b34585be21a64ab45ddcc2fb9f17be2dfeff6b22cf943bc3fc8f125a7f463af428ed0000000000
E         + 187ae0140470aaf1a975c27a541de1efbdb5f930ddcc6f3f1765dbd6547a24bba3dc34b682ca5f03e1426b75bee3e9009c92534865362000705f3415ab60d9e7a3e6cfce00000000000

tests/device_tests/bitcoin/test_signtx_taproot.py:285: AssertionError

-------- UI tests summary: --------
Run ./tests/show_results.py to open test summary

=============================================================================================== short test summary info ===============================================================================================
FAILED tests/device_tests/bitcoin/test_signtx_taproot.py::test_send_mixed - AssertionError: assert '010000000001...ce00000000000' == '010000000001...8ed0000000000'
============================================================================================= 1 failed, 7 passed in 7.84s =============================================================================================

[onvej-sl]

When I run the tests locally (after fixing the indentation), I have one failure:

Sorry for that. It seems to me that I committed an unfinished work. d3f5c59 fixes it.

Does the tests issue somehow involve the scriptPubKey check? I do not see the Andrew's commit enforcing this in that branch

This issue is not directly related to the scriptPubKey check.

[grdddj]

This issue is not directly related to the scriptPubKey check.

In the latest CI all the tests are green. Is there something I could help with?

[prusnak]

Is there something I could help with?

@onvej-sl if you are happy with the changes, will you please squash the fixup commits and create a PR so we can review and merge this?

[onvej-sl]

Is there something I could help with?

Definitely. Since signatures of some transactions were changed, the transactions don't correspond to the transactions on blockchain that are referenced in comments (see this). We probably need to create new transactions on blockchain for the tests that I modified. I was hoping it is something @grdddj has more experience with.

[grdddj]

Since signatures of some transactions were changed, the transactions don't correspond to the transactions on blockchain that are referenced in comments (see this). We probably need to create new transactions on blockchain for the tests that I modified.

Yes, seems like new blockchain transactions will be needed. We could also use the newly-created assert_tx_matches functionality to check whether the generated data really match those being accepted by blockchain (via blockbook).

I am not sure how to do Fujicoin and Groestlcoin transactions, but all the BTC testnet transactions should be alright.

Before I would do this, could we have a short meeting to discuss it?

[hynek-jina]

@onvej-sl
Please review.. thx

[bosomt]

@onvej-sl anything that needs to be tested on our side or I can move it from Needs QA section ?

[onvej-sl]

@onvej-sl anything that needs to be tested on our side or I can move it from Needs QA section ?

There is no need to test anything, thanks. I set the status to "Approved".