Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multisig fix #4351

Merged
merged 19 commits into from
Dec 2, 2024
Merged

Multisig fix #4351

merged 19 commits into from
Dec 2, 2024

Conversation

onvej-sl
Copy link
Contributor

@onvej-sl onvej-sl commented Nov 14, 2024

None of these changes were backported to legacy.

@onvej-sl onvej-sl force-pushed the onvej-sl/multisig-fix branch from 69da990 to 541c51c Compare November 14, 2024 13:59
Copy link

github-actions bot commented Nov 14, 2024

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)

Copy link

github-actions bot commented Nov 14, 2024

legacy UI changes device test(screens) main(screens)

@onvej-sl onvej-sl force-pushed the onvej-sl/multisig-fix branch 2 times, most recently from 6ad64e4 to 1f204aa Compare November 14, 2024 19:06
@onvej-sl onvej-sl marked this pull request as ready for review November 14, 2024 19:12
@onvej-sl onvej-sl added core Trezor Core firmware. Runs on Trezor Model T and T2B1. bitcoin Bitcoin related labels Nov 15, 2024
@onvej-sl
Copy link
Contributor Author

Related issue: #4356

@onvej-sl onvej-sl requested a review from mmilata November 18, 2024 13:56
@onvej-sl
Copy link
Contributor Author

@andrewkozlik please do a high-level review, ensuring particularly that this pull request fixes the mentioned issue.
@mmilata please do a generic review.

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, found some nits, another pair of eyes on apps/bitcoin/sign_tx/ would be nice.

core/.changelog.d/4351.added Outdated Show resolved Hide resolved
core/.changelog.d/4351.changed.2 Outdated Show resolved Hide resolved
core/failing_test Outdated Show resolved Hide resolved
core/src/apps/bitcoin/keychain.py Show resolved Hide resolved
python/.changelog.d/TODO.added Outdated Show resolved Hide resolved
legacy/firmware/transaction.c Outdated Show resolved Hide resolved
legacy/firmware/transaction.c Show resolved Hide resolved
legacy/firmware/transaction.c Show resolved Hide resolved
core/src/apps/bitcoin/sign_tx/matchcheck.py Outdated Show resolved Hide resolved
core/src/trezor/ui/layouts/mercury/__init__.py Outdated Show resolved Hide resolved
onvej-sl added a commit that referenced this pull request Nov 21, 2024
Co-authored-by: Martin Milata <[email protected]>
core/.changelog.d/4351.changed.1 Outdated Show resolved Hide resolved
core/src/apps/bitcoin/multisig.py Outdated Show resolved Hide resolved
core/src/apps/bitcoin/common.py Outdated Show resolved Hide resolved
core/src/apps/bitcoin/get_address.py Outdated Show resolved Hide resolved
core/.changelog.d/4351.changed.3 Outdated Show resolved Hide resolved
@onvej-sl onvej-sl mentioned this pull request Nov 27, 2024
Copy link
Contributor

@andrewkozlik andrewkozlik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High level this LGTM. Good job! Awaiting final approval from @mmilata.

@Hannsek we will need 3rd party multisig providers (Unchained, Casa, ...) to test that these changes do not break their setups. I believe you have the full set of contacts. Please coordinate with them before the next firmware release.

@onvej-sl onvej-sl force-pushed the onvej-sl/multisig-fix branch from b7d26c7 to 9c4e2f1 Compare November 29, 2024 15:28
@onvej-sl onvej-sl force-pushed the onvej-sl/multisig-fix branch 2 times, most recently from 6d4bd16 to b690807 Compare November 29, 2024 18:48
@onvej-sl onvej-sl force-pushed the onvej-sl/multisig-fix branch from b690807 to 7d4c0d2 Compare December 2, 2024 10:44
@onvej-sl onvej-sl merged commit e77477c into main Dec 2, 2024
95 checks passed
@onvej-sl onvej-sl deleted the onvej-sl/multisig-fix branch December 2, 2024 11:21
@onvej-sl onvej-sl mentioned this pull request Dec 2, 2024
@onvej-sl onvej-sl restored the onvej-sl/multisig-fix branch December 9, 2024 15:42
@andrewtoth
Copy link

andrewtoth commented Dec 10, 2024

Doesn't this break multisig_pubkey_index if using lexicographic order, since that will return the index of the pubkey if unsorted?

@onvej-sl
Copy link
Contributor Author

Doesn't this break multisig_pubkey_index if using lexicographic order, since that will return the index of the pubkey if unsorted?

I'm afraid you are right. It breaks the serialization of the multisig witness and signature. What is strange is that this test didn't catch it.

@andrewtoth
Copy link

That's because that test does not use that function in a breaking way. That is testing unsigned txs. The multisig_pubkey_index function breaks when it is used for inserting the signatures.

@andrewtoth
Copy link

I have it fixed in d066232

@onvej-sl
Copy link
Contributor Author

That's because that test does not use that function in a breaking way. That is testing unsigned txs. The multisig_pubkey_index function breaks when it is used for inserting the signatures.

I'm not sure if this is exactly what you're saying, but I discovered that the test fails to catch it because I use empty signatures in MultisigRedeemScriptType.

I have it fixed in d066232

The fix is not completely correct because multisig_pubkey_index is used here to detect which xpub belongs to you and the xpubs are not sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoin Bitcoin related core Trezor Core firmware. Runs on Trezor Model T and T2B1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants