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.

Sign up for GitHub

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

Jump to bottom

Fix bitcoin-only strings filtering #1633

Merged
merged 4 commits into from
Oct 7, 2021
Merged

Fix bitcoin-only strings filtering #1633

merged 4 commits into from
Oct 7, 2021

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented May 26, 2021

I propose merging this after core is made conformant, otherwise the core fw btconly build is gonna be red in every CI pipeline run.

Regarding legacy ISTM that the check should stay in the BITCOIN_ONLY, so only the error messages differ. Perhaps we can use the generic one even in full firmware?

@mmilata mmilata requested a review from prusnak as a code owner May 26, 2021 10:43
@mmilata mmilata requested a review from matejcik May 26, 2021 10:43
@matejcik
Copy link
Contributor

Regarding legacy ISTM that the check should stay in the BITCOIN_ONLY, so only the error messages differ. Perhaps we can use the generic one even in full firmware?

what would be the point of the check in BITCOIN_ONLY ?

I believe bitcoin-only firmware should ignore the existence of the field (iow, treat its value as if it were an unknown field), and protobuf messages containing unknown fields are valid (their values are ignored when decoding)

@mmilata
Copy link
Member Author

mmilata commented May 26, 2021

It wasn't immediately clear to me what allowing these additional valid inputs would do in the firmware. After looking at it a bit more I'm now quite sure the field is not used anywhere except 1 and 2 both of which check that we're processing decred tx in some way. Fixed in 2b5cd0a.

matejcik
matejcik approved these changes May 26, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

utACK

@mmilata mmilata mentioned this pull request Jun 22, 2021
Closed
@matejcik
Copy link
Contributor

ready to merge code-wise but depends on #1654

@mmilata mmilata force-pushed the mmilata/legacy-btconly branch from 2b5cd0a to 56ccab1 Compare September 28, 2021 21:54
@mmilata mmilata changed the title Fix bitcoin-only strings check, make legacy conform Fix bitcoin-only strings filtering Sep 28, 2021
@mmilata
Copy link
Member Author

mmilata commented Sep 28, 2021

Added cff1d5e that mostly fixes altcoins leaking into core by moving the include_in_bitcoin_only option from protobuf message level to file level where it is easier to mark all such messages/enums. Hopefully the import statements added to .proto defs are not a problem.

"Mostly fixes" means that there are still mentions of Decred. I'm tempted to allow it as an exception since it seems to be tightly bound to the bitcoin logic and separating it doesn't seem worthwhile.

Decred details provided but Decred coin not specified.
DecredStakingSpendType
)C#trezor.enums.DecredStakingSpendType
.Q&trezor/enums/DecredStakingSpendType.py
UiConfirmDecredSSTXSubmission
trezor/enums/DecredStakingSpendType.py

matejcik
matejcik approved these changes Oct 5, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nicely done.

I think keeping Decred is OK. We could export the Decred types into a separate messages-bitcoin-decred.proto file, and wrap mentions in if not BITCOIN_ONLY , but it's probably not worth the effort.

I'm surprised that the field names (decred_tree, decred_staking_spend_type) don't come up in the strings check.

Do you know why?

common/protob/messages-bootloader.proto Outdated
@@ -5,6 +5,10 @@ package hw.trezor.messages.bootloader;
option java_package = "com.satoshilabs.trezor.lib.protobuf";
option java_outer_classname = "TrezorMessageBootloader";

option (include_in_bitcoin_only) = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary? bootloader is not affected by BITCOIN_ONLY, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

True. Also webauthn doesn't belong in bitcoin-only fw.

@mmilata
Copy link
Member Author

mmilata commented Oct 6, 2021

I'm surprised that the field names (decred_tree, decred_staking_spend_type) don't come up in the strings check.

Very good point. Turns out the check was case-sensitive and the altcoin names are capitalized. In c465f21 I've added a whitelist which notably includes decred and omni due to the nontrivial integration into bitcoin code, and ripple and dash because they are BIP-39 words.

mmilata added 4 commits October 7, 2021 14:57
@mmilata
fix(legacy): don't mention decred in btconly fw
e47104c
@mmilata
ci: fix bitcoin-only strings check of fw images
0623bdf 
shellcheck saves the day

In tools/check-bitcoin-only line 9:
        RETURN=1
        ^----^ SC2030: Modification of RETURN is local (to subshell caused by pipeline).

In tools/check-bitcoin-only line 13:
exit $RETURN
     ^-----^ SC2031: RETURN was modified in a subshell. That change might be lost.
@mmilata
refactor(common/protob): make bitcoin_only options file-level
d460e93
@mmilata
fix(core): bitcoin-only all_modules.py
ee924c6
@mmilata mmilata force-pushed the mmilata/legacy-btconly branch from c7bcbf6 to ee924c6 Compare October 7, 2021 12:57
@mmilata mmilata merged commit 70bb0ee into master Oct 7, 2021
@mmilata mmilata deleted the mmilata/legacy-btconly branch October 7, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik matejcik approved these changes

@prusnak prusnak Awaiting requested review from prusnak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mmilata @matejcik