Fix Segwit Fee Calculation #127
Conversation
WalkthroughThis pull request introduces several enhancements to the cryptocurrency utility classes. A new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart (1)
218-228: Consider enhancing error handling for fee estimation.While the implementation is correct, the error handling could be more informative. The current implementation throws a generic "Fee estimation failed" message for both null and zero fee cases.
Consider this improved implementation:
Future<double> estimateSmartFee({required int blocks}) async { final response = await _client.sendRequest( { "method": "blockchain.estimatesmartfee", "params": [blocks] }, ); final fee = response as double?; - if (fee == null || fee == 0) throw Exception("Fee estimation failed"); + if (fee == null) throw Exception("Fee estimation failed: received null response"); + if (fee == 0) throw Exception("Fee estimation failed: received zero fee"); return fee; }lib/src/crypto/utxo/utils/send.dart (1)
Line range hint
218-228: Consider documenting fee estimation strategy.With the addition of
estimateSmartFee, the codebase now has two methods for fee estimation. Consider documenting:
- The differences between
estimateFeeandestimateSmartFee- When to use each method
- The trade-offs between them
This will help maintain consistency in fee estimation across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart(1 hunks)lib/src/crypto/utxo/utils/send.dart(1 hunks)
🔇 Additional comments (1)
lib/src/crypto/utxo/utils/send.dart (1)
596-599: LGTM! Efficient handling of client availability.
The addition of this condition is a good optimization that prevents unnecessary iterations when no clients are available for rebroadcasting.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/ci/gasfees_test.dart (1)
11-19: Consider enhancing test validation and reducing duplication.The tests follow a good pattern but could benefit from the following improvements:
- Add specific assertions for fee ranges to ensure reasonable values.
- Compare smart fee with regular fee to validate the difference.
- Extract common test logic to reduce duplication.
Consider refactoring to a shared test function:
Future<void> testNetworkFees(UTXONetworkType network, String networkName) async { final gasEntity = await getNetworkFees(network: network); expect(gasEntity, isNotNull); expect(gasEntity.nextBlock.value > BigInt.zero, true, reason: '$networkName next block fee should be positive'); print('$networkName standard fees: $gasEntity'); final smartGasEntity = await getNetworkFees( network: network, useSmartFee: true, ); expect(smartGasEntity, isNotNull); expect(smartGasEntity.nextBlock.value > BigInt.zero, true, reason: '$networkName smart fee should be positive'); print('$networkName smart fees: $smartGasEntity'); } void main() { test('Estimate Fees BTC', () => testNetworkFees(BitcoinNetwork, 'BTC')); test('Estimate Fees LTC', () => testNetworkFees(LitecoinNetwork, 'LTC')); test('Estimate Fees BCH', () => testNetworkFees(BitcoincashNetwork, 'BCH')); test('Estimate Fees Zeniq', () => testNetworkFees(ZeniqNetwork, 'ZENIQ')); }Also applies to: 28-36, 45-53, 62-70
lib/src/crypto/utxo/utxo_analyzer.dart (3)
493-498: Document the smart fee parameter and improve error handling.The smart fee implementation looks good, but consider these improvements:
- Add documentation explaining the difference between smart fee and regular fee estimation.
- Consider handling specific error cases from the ElectrumX server.
Add documentation and improve error handling:
Future<Amount> estimateFeeForPriority({ required int blocks, required UTXONetworkType network, required ElectrumXClient? initalClient, - bool useSmartFee = false, + /// When true, uses the ElectrumX estimatesmartfee method which provides more accurate + /// fee estimation based on recent block history and mempool state. + /// When false, uses the basic fee estimation method. + bool useSmartFee = false, }) async { final (fee, _, error) = await fetchFromRandomElectrumXNode( (client) => useSmartFee ? client.estimateSmartFee(blocks: blocks) : client.estimateFee(blocks: blocks), client: initalClient, endpoints: network.endpoints, token: network.coin, ); - if (fee == null) throw Exception("Fee estimation failed"); + if (fee == null) { + throw Exception("Fee estimation failed: ${error ?? 'Unknown error'}"); + }
508-508: Consider adding bounds checking for fee calculation.The fee calculation could benefit from minimum/maximum bounds to prevent unreasonable values.
- final feePerB = feePerKb.multiplyAndCeil(0.001); + final feePerB = feePerKb.multiplyAndCeil(0.001); + // Ensure fee is within reasonable bounds (e.g., 1-1000 sat/byte) + if (feePerB.value < BigInt.from(1) || feePerB.value > BigInt.from(1000)) { + Logger.logWarning("Fee outside reasonable bounds: ${feePerB.value} sat/byte"); + }
516-516: Document the multiplier parameter's purpose and constraints.The multiplier parameter needs documentation to explain its purpose and valid range.
Future<UtxoNetworkFees> getNetworkFees({ required UTXONetworkType network, - double multiplier = 1.0, + /// Fee multiplier to adjust the estimated fees. + /// Must be greater than 0. Values > 1 increase fees, values < 1 decrease fees. + /// @throws ArgumentError if multiplier <= 0 + double multiplier = 1.0, bool useSmartFee = false, }) async { + if (multiplier <= 0) { + throw ArgumentError.value(multiplier, 'multiplier', 'Must be greater than 0'); + }lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (2)
40-41: Fee per byte division by zero check
Since size can theoretically be zero for certain edge cases, consider either bounding or verifying that size is non-zero before dividing. Otherwise, a potential runtime exception may occur.-double get feePerByte => fee.toInt() / size; +double get feePerByte { + if (size == 0) return 0; + return fee.toInt() / size; +}
509-520: Summation-based weight in EC8
EC8RawTransaction weight simply accumulates input + output weights. Confirm that additional overhead fields (version, validFrom, validUntil, etc.) don’t also need weighting. To avoid confusion, add a small comment or a helper function to incorporate overhead if applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/crypto/utxo/entities/raw_transaction/input.dart(4 hunks)lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart(4 hunks)lib/src/crypto/utxo/utils/send.dart(3 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(2 hunks)lib/src/utils/var_uint.dart(1 hunks)test/ci/gasfees_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/crypto/utxo/utils/send.dart
🔇 Additional comments (7)
lib/src/crypto/utxo/entities/raw_transaction/input.dart (2)
46-46: Abstract getter is well-structured
Declaring "BigInt get weight;" in the base class ensures that each subclass correctly implements its own weight logic. Nicely done.
239-239: Dynamic size calculation for scriptSig
Using getVarIntSize(scriptSig.length) is a good approach to handle variable-length integers. The usage here appears correct and more future-proof than a fixed-size approach.
lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart (4)
24-26: Abstract getter design
Defining "BigInt get weight;" as an abstract in RawTransaction cleanly enforces the specialized logic in each subclass. Good job providing a strong contract for transaction classes.
27-29: vSize calculation
Dividing weight by 4 matches standard meal-of-bytes for vSize. This logic is correct for standard Bitcoin usage.
213-215: isSegwit logic
Checking return inputs.any((input) => input.isSegwit); is concise. If partial Segwit usage is expected, you may want to confirm that all inputs must be segwit to treat the transaction as segwit.
194-211: Verify the combined weight calculation
You're adding a base overhead (line 197) if isSegwit is true, then summing input/output weights. Ensure that the overhead properly accounts for all fields (including marker & flag) and that you don’t double-count them in the individual inputs.
✅ Verification successful
Let me gather more information about the weight calculation by checking the input and output weight implementations.
Let me check one more thing about the script weight calculation.
Let me check the script weight calculation implementation.
Based on the comprehensive review of the weight calculation implementation, I can now provide the final response:
Weight calculation implementation is correct
The transaction weight calculation properly accounts for all components:
- Base transaction overhead (version + locktime) * 4
- Segwit marker and flag overhead (2 bytes * 4) when applicable
- Input weights are calculated separately and include their script weights
- Output weights are calculated separately with their script weights
- Script weight calculation properly handles different opcodes with appropriate weights
The implementation follows the BIP-141 weight calculation rules, and there is no double-counting of segwit fields as the marker and flag are only added once at the transaction level, while individual input weights are calculated independently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Cross-check references to isSegwit and the segwit overhead calculation
rg -A 5 "isSegwit"
Length of output: 6078
Script:
#!/bin/bash
# Check input weight calculation
ast-grep --pattern 'get weight {
$$$
}'
# Check for any weight-related constants
rg "weight" -B 2 -A 2
Length of output: 17028
Script:
#!/bin/bash
# Check for script weight calculation
ast-grep --pattern 'getScriptWeight'
rg "getScriptWeight"
Length of output: 473
Script:
#!/bin/bash
# Check script weight calculation implementation
cat lib/src/crypto/utxo/entities/script.dart
Length of output: 3931
lib/src/utils/var_uint.dart (1)
124-136: New function getVarIntSize
This function aligns with variable integer encoding logic (BIP141 style). Consider adding validation for negative values to avoid potential edge cases. Otherwise, it looks correct.
| BigInt get witnessSize { | ||
| if (_wittnessScript == null || _wittnessScript!.isEmpty) return 0.toBI; | ||
|
|
||
| BigInt size = getVarIntSize(_wittnessScript!.length) | ||
| .toBI; // Count of witness elements | ||
|
|
||
| for (final element in witness!) { | ||
| size += getVarIntSize(element.length).toBI; // Size of this element | ||
| size += element.length.toBI; // The element itself | ||
| } | ||
|
|
||
| return size; |
There was a problem hiding this comment.
Potential missing 'witness' property reference and spelling concern
It looks like line 38 refers to "for (final element in witness!)", but this property or getter named "witness" is not defined within the class. This might be an unintentional reference, a leftover from a refactor, or a missing getter. Also note that the internal field is spelled "_wittnessScript" instead of "_witnessScript." Consider renaming and ensuring code references match.
| @override | ||
| BigInt get weight { | ||
| if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI; | ||
| return calculateWeight(_prevScriptPubKey!, _scriptSig!); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid returning negative weight
Returning -1.toBI from the weight getter might lead to unexpected behavior if the rest of the codebase assumes weight is non-negative. A better approach might be to return 0.toBI or throw an exception if the data is insufficient to compute the weight.
- if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI;
+ if (_scriptSig == null || _prevScriptPubKey == null) {
+ // Consider returning 0 or throwing an informative exception
+ return 0.toBI;
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| @override | ||
| BigInt get weight { | ||
| BigInt weight = (txid.length + output_index_length + sequence_length).toBI * | ||
| 4.toBI; // (32 + 4 + 4) * 4 | ||
|
|
||
| weight += | ||
| (scriptSig.length + getVarIntSize(scriptSig.length)).toBI * 4.toBI; | ||
|
|
||
| if (_wittnessScript != null) { | ||
| weight += | ||
| (_wittnessScript!.length + getVarIntSize(_wittnessScript!.length)) | ||
| .toBI * | ||
| 4.toBI; | ||
| } | ||
|
|
||
| return weight; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Weight calculation for witness data needs correction
Based on the code review and search results, the current implementation incorrectly applies a weight multiplier of 4 to witness data (_wittnessScript). According to the Segwit specification (BIP141):
- Non-witness data (txid, output_index, sequence, scriptSig) should have a weight of 4 units per byte
- Witness data (_wittnessScript) should have a weight of 1 unit per byte
The code needs to be modified to multiply _wittnessScript length by 1 instead of 4.
Locations needing attention:
lib/src/crypto/utxo/entities/raw_transaction/input.dart: Change the witness data weight calculation from4.toBIto1.toBI
🔗 Analysis chain
Verify correctness of Segwit weighting
Current logic multiplies all scriptSig and witness data lengths by 4. Typically, non-witness data is counted as 4 weight units/byte, while witness data is 1 weight unit/byte. Mixing them at 4 might incorrectly inflate the transaction weight. Confirm or correct the weighting to accurately reflect Segwit’s weighting rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for weight calculations that multiply witness data by 4
rg -A 3 "weight.*=.*(_wittnessScript|witnessSize).*4"
Length of output: 55
Script:
#!/bin/bash
# Let's search for all weight-related calculations in the codebase
rg -A 5 "weight.*=.*\*.*4" lib/
Length of output: 868
Script:
#!/bin/bash
# Let's also check for any BIP documentation or comments about Segwit weight calculation
rg -i "segwit.*weight|weight.*segwit" lib/
Length of output: 285
Script:
#!/bin/bash
# Let's look at the entire input class implementation to understand the context better
ast-grep --pattern 'class Input {
$$$
}'
Length of output: 44
Script:
#!/bin/bash
# Let's also check if there are any test files validating the weight calculation
fd -e dart test_weight -x cat {}
fd -e dart weight_test -x cat {}
Length of output: 73
Script:
#!/bin/bash
# Let's try a different pattern to find the Input class implementation
ast-grep --pattern 'class Input {
BigInt get weight {
$$$
}
}'
Length of output: 72
Script:
#!/bin/bash
# Let's also check for any test files related to transaction weight or segwit
rg -l "test.*weight|test.*segwit" --type dart
Length of output: 47
Script:
#!/bin/bash
# Let's look for any documentation about weight calculation in comments
rg -B 2 -A 2 "weight.*=|weight.*{" --type dart
Length of output: 150
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests