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

Make SIGHASH_FORKID configurable with coins file #912

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Make SIGHASH_FORKID configurable with coins file #912

merged 3 commits into from
Apr 21, 2021

Conversation

shamardy
Copy link
Collaborator

fixes #905

artemii235
artemii235 previously approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Great, thanks for the PR!
@cipig Could you please test that LCC (Litecoincach) works with these changes?

cipig
cipig previously approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@cipig cipig left a comment

Choose a reason for hiding this comment

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

LCC now works fine: https://chainz.cryptoid.info/lcc/tx.dws?8dee0a26b4f5cc13084a5049140b9df27318568107f85ff6cf2a6c3f578356c0
Thanks for the fix.

@artemii235 artemii235 self-requested a review April 20, 2021 12:19
artemii235
artemii235 reviewed Apr 20, 2021
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

@shamardy I have just noticed that for the backward-compatibility purpose it's better to keep hard-code for BCH as follows:
If fork_id is set use it
If not set use 0x40 for BCH and 0x0 for all other coins

We might be in a situation when MM2 is updated but the coins config is not. So BCH transaction signing will be broken. The proposed change will mitigate this. Could you add it, please?

@shamardy shamardy dismissed stale reviews from cipig and artemii235 via 7f53032 April 20, 2021 14:43
cipig
cipig approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@cipig cipig left a comment

Choose a reason for hiding this comment

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

LCC swaps still work fine after last changes

artemii235
artemii235 approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Great, thanks for PR and testing 🙂

@artemii235 artemii235 merged commit 2b3db4f into KomodoPlatform:mm2.1 Apr 21, 2021
@artemii235
Copy link
Member

@cipig Could you update the coins repo, please?

@cipig
Copy link
Member

cipig commented Apr 21, 2021

already done, BCH and LCC now have fork_id set in coins repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipig cipig cipig approved these changes

@artemii235 artemii235 artemii235 approved these changes

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

Successfully merging this pull request may close these issues.

Make SIGHASH_FORKID configurable through the coins file.
3 participants
@shamardy @artemii235 @cipig