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

Electrum indexer refactoring and bug fixing #63

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Electrum indexer refactoring and bug fixing #63

merged 2 commits into from
Aug 26, 2024

Conversation

dr-orlovsky
Copy link
Member

@dr-orlovsky dr-orlovsky commented Aug 23, 2024

Alternative to #61 which additionally fixes other bugs and issues with the code:

  • consensus deserialization which instead of binary bytes takes a byte representation of the hex string, resulting in invalid transaction data:
    let bp_tx = Tx::consensus_deserialize(hex_bytes)
  • panics due to expects in all data processings, which will result in global crashing and DDoS for all RGB wallets if a major electrum server used by them would be compromised or will have an API break

It also removes overall convolution of the logic which makes it hard to review and audit

The PR leaves further required and important improvements for the later: #64

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Aug 23, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Aug 23, 2024
@dr-orlovsky dr-orlovsky requested review from nicbus and zoedberg August 23, 2024 08:26
@dr-orlovsky dr-orlovsky self-assigned this Aug 23, 2024
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 98 lines in your changes missing coverage. Please review.

Project coverage is 0.0%. Comparing base (dbfb8ba) to head (2047ef9).
Report is 3 commits behind head on master.

Files Patch % Lines
src/indexers/electrum.rs 0.0% 96 Missing ⚠️
src/indexers/esplora.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##           master    #63   +/-   ##
=====================================
  Coverage     0.0%   0.0%           
=====================================
  Files          23     23           
  Lines        2128   2071   -57     
=====================================
+ Misses       2128   2071   -57     
Flag Coverage Δ
rust 0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dr-orlovsky
Copy link
Member Author

dr-orlovsky commented Aug 23, 2024

consensus deserialization which instead of binary bytes takes a byte representation of the hex string, resulting in invalid transaction data:

let bp_tx = Tx::consensus_deserialize(hex_bytes)

I am sorry, this is actually not true, I misread the previous code: the hex_bytes are binary coming out of Vec::from_hex

But still it is better to use simpler Tx::from_str form.

@dr-orlovsky
Copy link
Member Author

I have checked integration tests, they all do pass

Copy link
Contributor

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

I confirm all integration tests are working.

@dr-orlovsky dr-orlovsky merged commit ed03c02 into master Aug 26, 2024
25 of 29 checks passed
@dr-orlovsky dr-orlovsky deleted the fix/62 branch January 1, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

non-mandatory-script-verify-flag error when broadcasting tx with electrum
2 participants