Multisig fix in legacy

[onvej-sl]

This pull request backports #4351 to legacy.

[github-actions]

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3B1 Safe 3	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3T1 Safe 5	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

[mmilata]

• fixtures are missing (tests/update_fixtures.py ci should work)
• I'm getting some failures on hardware that I'm not seeing in emulator, e.g.:

  FAILED tests/device_tests/bitcoin/test_multisig_change.py::test_sorted_multisig_change_match_first - AssertionError: Expected responses:
  FAILED tests/device_tests/bitcoin/test_multisig_change.py::test_multisig_change_match_second - AssertionError: Expected responses:
  FAILED tests/device_tests/bitcoin/test_multisig_change.py::test_multisig_change_match_first - AssertionError: Expected responses:
  FAILED tests/device_tests/bitcoin/test_bcash.py::test_send_bch_multisig_change - AssertionError: Expected responses:
  

  Not 100% sure this is because of this PR, would appreciate someone else checking. Needs fix(legacy): uint64 protobuf field decoding #4535 in order to work on HW.

[onvej-sl]

I'm getting some failures on hardware that I'm not seeing in emulator, e.g.:

I hopefully fixed it in bc0ecfe.

[onvej-sl]

I squashed the fixup commits, rebased on top of the main branch and added fixtures.

[onvej-sl]

I'm getting some failures on hardware that I'm not seeing in emulator, e.g.:

I hopefully fixed it in bc0ecfe.

My trezor one device is somewhat broken so I was only able to run tests/device_tests/bitcoin/test_multisig_change.py and tests/device_tests/bitcoin/test_bcash.py and both ran successfully.

[mmilata]

Can confirm that all multisig tests now pass on my T1. But I don't understand where the problem was - shouldn't multisig->pubkeys_order be all zeros when not present in the message? Is this related to -fshort-enums?

[onvej-sl]

To be honest, I'm not certain. However, based on how the code is carefully written (see, for example, this), I thought that property is uninitialized if has_property is false.

EDITED: Changed example.

[prusnak]

I think the property is set to zeroes if unset by wire, but still I prefer to have the code explicit rather than implicit to be on the safe side once this behaviour changes in future versions of nanopb or something like that.

Is this related to -fshort-enums?

Also this might be the culprit in some cases like we've seen recently. So explicit code is better.

[mmilata]

Yep I think it's -fshort-enums, on hardware sizeof(MultisigPubkeysOrder) == 1 so sha256_Update((const uint8_t *)&(multisig->pubkeys_order), sizeof(uint32_t)) reads also the next member in the struct, and it's not 0. Emulator doesn't use that option so it works there.

[matejcik]

...that means that this code is wrong, right?
what we want is, pseudocode:

MultisigPubkeysOrder pubkeys_order = ...
sha256_Update(multisig->m, sizeof(multisig->m);
sha256_Update(pubkeys_order, sizeof(pubkeys_order));
...

etc for this whole block.

[onvej-sl]

I believe the code is correct now. The only difference from your pseudocode is that I cast MultisigPubkeysOrder to uint32_t.

[matejcik]

my point is that we're magically relying on all the things being u32-sized, with no actual proof in the code (which was the source of the previous bug)

the code is correct now, but could break again in theory

[onvej-sl]

I have no problem changing uint32_t to MultisigPubkeysOrder. However, I don't still understand what you mean by "but could break again in theory". My code doesn't rely on MultisigPubkeysOrder to be uint32_t.

[onvej-sl]

In this case, it is not important, but generally, I think my solution is better because it ensures that the resulting hash is the same on both the emulator and the hardware.

[matejcik]

#4547

[mmilata]

I think both the requirement that the hash result stays stable as well as resilience to member size changes are valid.

From this POV it would be best to go with

sha256_Update(multisig->m, sizeof(multisig->m));

followed by a static assert that sizeof(multisig->m) == sizeof(uint32_t). As it's a matter of taste I'd say let's merge this PR as it is so we don't hold up the release, and continue in another PR.

[onvej-sl]

Okay, I understand now. I initially thought you were only referring to pubkeys_order, but you are also talking about m and others.

[onvej-sl]

I tested it and can confirm that the size of MultisigPubkeysOrder is 4 in the emulator, while it is 1 on the hardware.