-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for Litecoin (LTC) and Bitcoin Cash (BCH) blockchain indexing to the codebase. Both implementations closely mirror the existing Bitcoin chain implementation structure. Note that the PR description indicates the BCH version fails on processing blocks since MWEB activation, which requires patching the litcoin crate.
Key Changes
- Complete Litecoin chain implementation with REST API client, block processing, address codec support (Base58Check and Bech32), and configuration
- Complete Bitcoin Cash chain implementation with HTTP/RPC client, block processing, legacy address support, and configuration
- CI integration for testing and benchmarking both new chains
Reviewed Changes
Copilot reviewed 28 out of 32 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| chains/ltc/src/rest_client.rs | REST client for fetching blocks from Litecoin node |
| chains/ltc/src/model_v1.rs | Data model definitions for LTC blockchain entities |
| chains/ltc/src/main.rs | Entry point for LTC chain indexer |
| chains/ltc/src/lib.rs | Library setup with error types and config |
| chains/ltc/src/hook.rs | Transaction input processing hooks |
| chains/ltc/src/codec.rs | Address encoding/decoding for Litecoin addresses |
| chains/ltc/src/block_provider.rs | Block processing and parsing logic |
| chains/ltc/config/*.toml | Configuration files for LTC node and indexer |
| chains/ltc/Cargo.toml | Dependencies for Litecoin implementation |
| chains/ltc/benches/ltc_benchmark.rs | Performance benchmarks |
| chains/ltc/README.md | Setup and usage documentation |
| chains/bch/src/http_client.rs | HTTP/RPC client for fetching blocks from BCH node |
| chains/bch/src/model_v1.rs | Data model definitions for BCH blockchain entities |
| chains/bch/src/main.rs | Entry point for BCH chain indexer |
| chains/bch/src/lib.rs | Library setup with error types and config |
| chains/bch/src/hook.rs | Transaction input processing hooks |
| chains/bch/src/codec.rs | Address encoding/decoding for BCH legacy addresses |
| chains/bch/src/block_provider.rs | Block processing and parsing logic |
| chains/bch/config/*.toml | Configuration files for BCH node and indexer |
| chains/bch/Cargo.toml | Dependencies for Bitcoin Cash implementation |
| chains/bch/benches/bch_benchmark.rs | Performance benchmarks |
| chains/bch/README.md | Setup and usage documentation |
| Cargo.toml | Added ltc and bch to workspace members |
| .github/workflows/redbit.yml | Added CI test and bench jobs for both chains |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chains/ltc/config/settings.toml
Outdated
| name = "ltc" | ||
| db_path = "/opt/.chain" | ||
| enable = true | ||
| node_sync_interval_s = 1 # 0 means only chatching up but no periodic sync |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "chatching" should be "catching"
| node_sync_interval_s = 1 # 0 means only chatching up but no periodic sync | |
| node_sync_interval_s = 1 # 0 means only catching up but no periodic sync |
chains/bch/config/settings.toml
Outdated
| name = "bch" | ||
| db_path = "/opt/.chain" | ||
| enable = true | ||
| node_sync_interval_s = 1 # 0 means only chatching up but no periodic sync |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "chatching" should be "catching"
| node_sync_interval_s = 1 # 0 means only chatching up but no periodic sync | |
| node_sync_interval_s = 1 # 0 means only catching up but no periodic sync |
chains/bch/config/settings.toml
Outdated
| fork_detection_heights = 100 # max 255, how many blocks from tip to check for forks | ||
| db_cache_size_gb = "mild" # off / tiny / low / mild / high / ultra | ||
| min_entity_batch_size = 1_000_000 # Sum of : inputs + outputs | ||
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in MB to buffer |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent units in comment: The comment states "total size of fetched blocks in MB to buffer" but the field name is max_entity_buffer_kb_size (KB). Either the comment should say "KB" or the field should be renamed to use MB units.
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in MB to buffer | |
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in KB to buffer |
chains/ltc/src/hook.rs
Outdated
| let id = TransactionPointer::from_parent(*id_pointer, *id_index as u16); | ||
| let utxo_pointer = match tx_pointer_buf_opt { | ||
| Some(tx_pointer_buf) => TransactionPointer::from_parent(tx_pointer_buf.as_value(), input_ref.index), | ||
| // coinbase marker, actually u32::MAX, but we store btc input index as u16 as it never exceeds that in by size limits |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment contains "btc" instead of "ltc": The comment says "we store btc input index as u16" but this is the Litecoin implementation. Update to "litecoin" or "ltc" for consistency.
| // coinbase marker, actually u32::MAX, but we store btc input index as u16 as it never exceeds that in by size limits | |
| // coinbase marker, actually u32::MAX, but we store ltc input index as u16 as it never exceeds that by size limits |
|
|
||
| pub const TAG_SEGWIT: u8 = 0xB0; | ||
| pub const TAG_OP_RETURN: u8 = 0xF0; |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers should be documented: The constants TAG_SEGWIT (0xB0), TAG_OP_RETURN (0xF0), and TAG_NON_ADDR (0xFF) are defined but would benefit from inline documentation explaining their purpose and why these specific values were chosen.
| pub const TAG_SEGWIT: u8 = 0xB0; | |
| pub const TAG_OP_RETURN: u8 = 0xF0; | |
| /// Tag for SegWit (Bech32) addresses. 0xB0 is chosen to distinguish SegWit payloads from legacy types. | |
| pub const TAG_SEGWIT: u8 = 0xB0; | |
| /// Tag for OP_RETURN outputs. 0xF0 is used to mark data-carrying outputs (not spendable addresses). | |
| pub const TAG_OP_RETURN: u8 = 0xF0; | |
| /// Tag for non-address payloads. 0xFF is reserved to indicate an invalid or non-address type. |
chains/bch/README.md
Outdated
| export BITCOINCASH__rpc_user = "foo" | ||
| export BITCOINCASH__rpc_password = "bar" |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid environment variable syntax: Remove spaces around the = sign. The correct syntax should be export BITCOINCASH__rpc_user="foo" (no spaces).
| export BITCOINCASH__rpc_user = "foo" | |
| export BITCOINCASH__rpc_password = "bar" | |
| export BITCOINCASH__rpc_user="foo" | |
| export BITCOINCASH__rpc_password="bar" |
chains/ltc/config/settings.toml
Outdated
| fork_detection_heights = 100 # max 255, how many blocks from tip to check for forks | ||
| db_cache_size_gb = "mild" # off / tiny / low / mild / high / ultra | ||
| min_entity_batch_size = 1_000_000 # Sum of : inputs + outputs | ||
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in MB to buffer |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent units in comment: The comment states "total size of fetched blocks in MB to buffer" but the field name is max_entity_buffer_kb_size (KB). Either the comment should say "KB" or the field should be renamed to use MB units.
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in MB to buffer | |
| max_entity_buffer_kb_size = 8192 # total size of fetched blocks in KB to buffer |
| let ver = src[1]; | ||
| let prog = &src[2..]; | ||
| if prog.len() != 20 && prog.len() != 32 { | ||
| return Err(serde::ser::Error::custom("invalid segwit program length")); |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message could be more helpful: "invalid segwit program length" doesn't specify what valid lengths are. Consider: "invalid segwit program length (expected 20 or 32 bytes)".
| return Err(serde::ser::Error::custom("invalid segwit program length")); | |
| return Err(serde::ser::Error::custom("invalid segwit program length (expected 20 or 32 bytes)")); |
| .map_err(|e| serde::de::Error::custom(format!("Bech32 parse failed: {e}")))?; | ||
| let _ = hrp; // HRP not stored in index bytes | ||
| if prog.len() != 20 && prog.len() != 32 { | ||
| return Err(serde::de::Error::custom("invalid segwit program length")); |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message could be more helpful: "invalid segwit program length" doesn't specify what valid lengths are. Consider: "invalid segwit program length (expected 20 or 32 bytes)".
| return Err(serde::de::Error::custom("invalid segwit program length")); | |
| return Err(serde::de::Error::custom("invalid segwit program length (expected 20 or 32 bytes)")); |
| rpc_user = "" # change this either in .env file or export is as env var | ||
| rpc_password = "" No newline at end of file |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: The configuration file contains empty RPC credentials with an instruction to change them, but this could lead to accidental deployment with empty credentials. Consider either providing secure placeholder values or making these required fields that fail on startup if empty.
| rpc_user = "" # change this either in .env file or export is as env var | |
| rpc_password = "" | |
| rpc_user = "CHANGEME_USER" # MUST change this in .env file or export as env var before deployment | |
| rpc_password = "CHANGEME_PASSWORD" # MUST change this in .env file or export as env var before deployment |
Adding support for LTC and BCH
Note that current BCH version fails on processing blocks since MWEB activation which would require patching the litcoin crate. It fails right at the start because chain indexer processes the tip block header.