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

Implementation of RSKIP379 #2186

Merged
merged 122 commits into from
Dec 12, 2023
Merged

Conversation

marcos-iov
Copy link
Contributor

@marcos-iov marcos-iov commented Nov 22, 2023

Create an index in the Bridge to store the sighash of transactions created by the Bridge. This index can then be used to simplify the detection of peg-in/peg-out/migration transactions.

fed:bridge-btc-tx-index-integration
rit:adds-tbd600-fork

@marcos-iov marcos-iov requested a review from a team as a code owner November 22, 2023 19:23
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@marcos-iov marcos-iov force-pushed the bridge-btc-tx-index-integration branch from 767dbe9 to e96d03b Compare November 23, 2023 15:58
@marcos-iov
Copy link
Contributor Author

pipeline:run

@marcos-iov marcos-iov force-pushed the bridge-btc-tx-index-integration branch from 1cfdc9c to aa42ace Compare December 6, 2023 17:26
…onfigsForTest helper

- Got rid of sigHash parameter when testing null values
- Got rid of no needed comments
- Renamed BRIDGE_BTC_TX_SIG_HASH_KEY to BRIDGE_BTC_TX_SIG_HASH
- Renamed test parameter provider's methods
- Added tests testing invalid stored data, considered an illegal state of the storage
- Created a copy of PegTestUtils.CreateHash method into BitcoinTestUtils.
- Replaced usage of PegTestUtils.CreateHash method to BitcoinTestUtils.CreateHash
Added test that assert redeemScript can be obtained from input.
Made createBaseInputScriptThatSpendsFromTheFederation and createBaseRedeemScriptThatSpendsFromTheFederation methods simpler.
Made BridgeUtils.extractRedeemScriptFromInput public.
Refactored BitcoinUtils.getFirstInputSigHash and tests to get redeemscript using BridgeUtils.extractRedeemScriptFromInput.
Reorganized tests.
- Moved common init test code to a beforeEach method.
- Added new test using a real btc transaction and checking extracted sigHash against the signature. Experiment worked.
- Used ScriptBuilder to get p2SHScript.
- Refactored sighash verification.
- Renamed method to extractSignaturesFromTxInput.
Improved assertions at BitcoinUtilsTest.
Refactored helper methods.
@marcos-iov marcos-iov force-pushed the bridge-btc-tx-index-integration branch from ef66415 to edda853 Compare December 11, 2023 14:52
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

sonarcloud bot commented Dec 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 26 Code Smells

97.6% 97.6% Coverage
0.0% 0.0% Duplication

@marcos-iov
Copy link
Contributor Author

pipeline:run

8 similar comments
@marcos-iov
Copy link
Contributor Author

pipeline:run

@marcos-iov
Copy link
Contributor Author

pipeline:run

@marcos-iov
Copy link
Contributor Author

pipeline:run

@nathanieliov
Copy link
Contributor

pipeline:run

@nathanieliov
Copy link
Contributor

pipeline:run

@nathanieliov
Copy link
Contributor

pipeline:run

@marcos-iov
Copy link
Contributor Author

pipeline:run

@marcos-iov
Copy link
Contributor Author

pipeline:run

Copy link
Contributor

@nathanieliov nathanieliov left a comment

Choose a reason for hiding this comment

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

LGTM

@marcos-iov
Copy link
Contributor Author

pipeline:run

2 similar comments
@marcos-iov
Copy link
Contributor Author

pipeline:run

@marcos-iov
Copy link
Contributor Author

pipeline:run

@josedahlquist josedahlquist merged commit 38e1212 into master Dec 12, 2023
11 checks passed
@josedahlquist josedahlquist deleted the bridge-btc-tx-index-integration branch December 12, 2023 13:50
@aeidelman aeidelman added this to the Arrowhead 6.0.0 milestone Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants