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

Fix Segwit Fee Calculation #127

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix Segwit Fee Calculation #127

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1623b4c
fix: prevent rebroadcasting when no clients are available
dev2-nomo Dec 19, 2024
dfcac37
feat: add smart fee estimation method to ElectrumXClient
dev2-nomo Dec 19, 2024
30719b8
Start with Weight Calculation
dev2-nomo Dec 19, 2024
93ff42c
Merge branch 'main' of https://github.com/nomo-app/walletkit-dart int…
dev2-nomo Feb 13, 2025
8a86a6f
refactor: improve weight calculation and script size handling in BTCI…
dev2-nomo Feb 13, 2025
50e9371
Initial Refactor for UTXOScript and TransactionStructure
dev2-nomo Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions lib/src/crypto/utxo/entities/raw_transaction/input.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,22 @@ sealed class Input {
_prevScriptPubKey = prevScriptPubKey,
_wittnessScript = wittnessScript;

BigInt get weight {
if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI;
return calculateWeight(_prevScriptPubKey!, _scriptSig!);
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;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

}

BigInt get weight;

int get intValue => value != null ? value!.toInt() : 0;

String? get scriptSigHex => _scriptSig != null ? _scriptSig!.toHex : null;
Expand Down Expand Up @@ -225,7 +236,7 @@ class BTCInput extends Input {
txid.length +
output_index_length +
scriptSig.length +
1 +
getVarIntSize(scriptSig.length) +
sequence_length,
);

Expand All @@ -244,6 +255,24 @@ class BTCInput extends Input {

return buffer;
}

@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;
}
Copy link

Choose a reason for hiding this comment

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

💡 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 from 4.toBI to 1.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

}

const value_length = 8;
Expand All @@ -259,6 +288,12 @@ class EC8Input extends Input {
super.wittnessScript,
});

@override
BigInt get weight {
if (_scriptSig == null || _prevScriptPubKey == null) return -1.toBI;
return calculateWeight(_prevScriptPubKey!, _scriptSig!);
}

Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

EC8Input addScript({
Uint8List? scriptSig,
Uint8List? wittnessScript,
Expand Down
52 changes: 43 additions & 9 deletions lib/src/crypto/utxo/entities/raw_transaction/raw_transaction.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@ sealed class RawTransaction {
/// Non Null if returned from [buildUnsignedTransaction]
final Map<ElectrumOutput, Input>? inputMap;

BigInt get weight {
return inputs.fold(
0.toBI,
(prev, input) => prev + input.weight,
) +
outputs.fold(
0.toBI,
(prev, output) => prev + output.weight,
);
/// Weight of the transaction
BigInt get weight;

/// Virtual Size
BigInt get vSize {
return weight ~/ 4.toBI;
}

Uint8List get bytes;
Expand All @@ -40,6 +37,8 @@ sealed class RawTransaction {

BigInt get fee => totalInputValue - totalOutputValue;

double get feePerByte => fee.toInt() / size;

// Value of the first output
BigInt get targetAmount => outputs.first.value;

Expand Down Expand Up @@ -192,6 +191,29 @@ class BTCRawTransaction extends RawTransaction {
outputs: outputs,
);

@override
BigInt get weight {
// Base size * 4
BigInt weight = 8.toBI * 4.toBI; // version + locktime

if (isSegwit) {
weight += 2.toBI * 4.toBI; // Segwit Marker + Segwit Flag
}

return inputs.fold(
0.toBI,
(prev, input) => prev + input.weight,
) +
outputs.fold(
0.toBI,
(prev, output) => prev + output.weight,
);
}

bool get isSegwit {
return inputs.any((input) => input.isSegwit);
}

bool get hasWitness {
return inputs.any((input) => input.hasWitness);
}
Expand Down Expand Up @@ -484,6 +506,18 @@ class EC8RawTransaction extends RawTransaction {
outputs: outputs,
);

/// EC8 Transaction weight is calculated differently
BigInt get weight {
return inputs.fold(
0.toBI,
(prev, input) => prev + input.weight,
) +
outputs.fold(
0.toBI,
(prev, output) => prev + output.weight,
);
}

String get txid {
final buffer = bytesForTxId;
final hash = sha256Sha256Hash(buffer);
Expand Down
12 changes: 12 additions & 0 deletions lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,18 @@ class ElectrumXClient {
return fee;
}

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");
return fee;
}

Future<bool> disconnect() async {
await _client.disconnect();
return true;
Expand Down
11 changes: 9 additions & 2 deletions lib/src/crypto/utxo/utils/send.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,6 @@ RawTransaction buildUnsignedTransaction({
"Total Input Value does not match Total Output Value",
);

Logger.log("Estimated Fee: $estimatedFee");

final outputs = buildOutputs(
recipient: targetAddress,
value: targetValue,
Expand All @@ -186,6 +184,10 @@ RawTransaction buildUnsignedTransaction({
"Total Output Value does not match Total Input Value",
);
}
Logger.log("Input Fee per Byte: ${feePerByte.displayDouble}");
Logger.log("Estimated Fee: $estimatedFee");
Logger.log("Actual Fee: ${tx.fee}");
Logger.log("Fee per Byte: ${tx.feePerByte}");

return tx;
}
Expand Down Expand Up @@ -598,6 +600,10 @@ Future<bool> rebroadcastTransaction({
],
);

if (clientsForRebroadcast.isEmpty) {
break;
}

if (rebroadcastCount > type.endpoints.length / 2) {
break;
}
Expand Down Expand Up @@ -684,6 +690,7 @@ BigInt calculateFee({
}) {
return switch (tx) {
EC8RawTransaction _ => calculateFeeEC8(tx: tx),
BTCRawTransaction tx when tx.isSegwit => tx.weight * feePerByte.value,
_ => tx.size.toBI * feePerByte.value,
};
}
Expand Down
8 changes: 6 additions & 2 deletions lib/src/crypto/utxo/utxo_analyzer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,9 +494,12 @@ Future<Amount> estimateFeeForPriority({
required int blocks,
required UTXONetworkType network,
required ElectrumXClient? initalClient,
bool useSmartFee = false,
}) async {
final (fee, _, _) = await fetchFromRandomElectrumXNode(
(client) => client.estimateFee(blocks: blocks),
(client) => useSmartFee
? client.estimateSmartFee(blocks: blocks)
: client.estimateFee(blocks: blocks),
client: initalClient,
endpoints: network.endpoints,
token: network.coin,
Expand All @@ -506,14 +509,15 @@ Future<Amount> estimateFeeForPriority({

final feePerKb = Amount.convert(value: fee, decimals: 8);

final feePerB = feePerKb / Amount.from(value: 1000, decimals: 0);
final feePerB = feePerKb.multiplyAndCeil(0.001);

return feePerB;
}

Future<UtxoNetworkFees> getNetworkFees({
required UTXONetworkType network,
double multiplier = 1.0,
bool useSmartFee = false,
}) async {
final blockInOneHour = 3600 ~/ network.blockTime;
final blocksTillTomorrow = 24 * 3600 ~/ network.blockTime;
Expand Down
13 changes: 13 additions & 0 deletions lib/src/utils/var_uint.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,16 @@ int encodingLength(int number) {
? 5
: 9);
}

int getVarIntSize(int value) {
if (value < 0xfd) {
return 1;
}
if (value <= 0xffff) {
return 3;
}
if (value <= 0xffffffff) {
return 5;
}
return 9;
}
36 changes: 36 additions & 0 deletions test/ci/fetching/gasfees_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ void main() {
expect(gasEntity, isNotNull);

print(gasEntity);

final smartGasEntity = await getNetworkFees(
network: BitcoinNetwork,
useSmartFee: true,
);

expect(smartGasEntity, isNotNull);

print(smartGasEntity);
});

test('Estimate Fees LTC', () async {
Expand All @@ -16,6 +25,15 @@ void main() {
expect(gasEntity, isNotNull);

print(gasEntity);

final smartGasEntity = await getNetworkFees(
network: LitecoinNetwork,
useSmartFee: true,
);

expect(smartGasEntity, isNotNull);

print(smartGasEntity);
});

test('Estimate Fees BCH', () async {
Expand All @@ -24,6 +42,15 @@ void main() {
expect(gasEntity, isNotNull);

print(gasEntity);

final smartGasEntity = await getNetworkFees(
network: BitcoincashNetwork,
useSmartFee: true,
);

expect(smartGasEntity, isNotNull);

print(smartGasEntity);
});

test('Estimate Fees Zeniq', () async {
Expand All @@ -32,5 +59,14 @@ void main() {
expect(gasEntity, isNotNull);

print(gasEntity);

final smartGasEntity = await getNetworkFees(
network: ZeniqNetwork,
useSmartFee: true,
);

expect(smartGasEntity, isNotNull);

print(smartGasEntity);
});
}
Loading