wallet: resolve root cause of broken zero-conf and inflated confirmations in GetTransaction API
#1114
Conversation
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Nice catch! Let's also make a PR in the lnd repo to test out this change?
wallet/wallet.go
Outdated
|
|
||
| bestBlock := w.SyncedTo() | ||
| blockHeight := txDetail.Block.Height | ||
| res.Confirmations = bestBlock.Height - blockHeight + 1 |
There was a problem hiding this comment.
We should use confirms here
There was a problem hiding this comment.
We should use
confirmshere
To be honest, The current function names confirms and confirmed, are not descriptive of their actions and are hard to recall. Renamed them to calculateConfirmations and hasMinimumConfirmations respectively
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
from
4263f16 to
4daad19
Compare
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
3 times, most recently
from
5b56413 to
6771222
Compare
The LND PR: lightningnetwork/lnd#10364 |
|
After this is merged into btcwallet master branch we need to cross merge it as well in |
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
from
6771222 to
94de4bb
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Looking good, the only comment is that we can try to make the method names a bit shorter without losing the semantics.
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
from
94de4bb to
b98ee9b
Compare
Adding regression test for bug where GetTransaction incorrectly calculated confirmations. The bug set confirmations to block height instead of calculating as (currentHeight - blockHeight + 1). It is mainly utilizing `calcConf`.
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
3 times, most recently
from
3382028 to
2ad982d
Compare
Fix bug where GetTransaction was setting confirmations to the block height instead of calculating them correctly.
mohamedawnallah
force-pushed
the
fix-gettransaction-confirmations
branch
from
2ad982d to
a04884f
Compare
|
Here is the code diff between the last approval and time of posting that commit. Overall, we need to be explicit in setting default fields: diff --git a/wallet/wallet.go b/wallet/wallet.go
index 7a08c1b..a30958a 100644
--- a/wallet/wallet.go
+++ b/wallet/wallet.go
@@ -2607,14 +2607,17 @@ func (w *Wallet) GetTransaction(txHash chainhash.Hash) (*GetTransactionResult,
res = GetTransactionResult{
Summary: makeTxSummary(dbtx, w, txDetail),
- Timestamp: txDetail.Block.Time.Unix(),
+ BlockHash: nil,
+ Height: -1,
Confirmations: 0,
+ Timestamp: 0,
}
// If it is a confirmed transaction we set the corresponding
- // block height and hash.
+ // block height, timestamp, hash, and confirmations.
if txDetail.Block.Height != -1 {
res.Height = txDetail.Block.Height
+ res.Timestamp = txDetail.Block.Time.Unix()
res.BlockHash = &txDetail.Block.Hash
bestBlock := w.SyncedTo()
diff --git a/wallet/wallet_test.go b/wallet/wallet_test.go
index 1b768dc..527cd83 100644
--- a/wallet/wallet_test.go
+++ b/wallet/wallet_test.go
@@ -247,8 +247,9 @@ func TestGetTransaction(t *testing.T) {
expectedErr error
}{
{
- name: "existing unmined transaction",
- txid: *TstTxHash,
+ name: "existing unmined transaction",
+ txid: *TstTxHash,
+ expectedHeight: -1,
// We write txdetail for the tx to disk.
f: func(s *wtxmgr.Store, ns walletdb.ReadWriteBucket) (
*wtxmgr.Store, error) {
@@ -345,13 +346,17 @@ func TestGetTransactionConfirmations(t *testing.T) {
// Expected height in result.
expectedHeight int32
+
+ // Whether to check for non-zero timestamp.
+ expectTimestamp bool
}{
{
name: "unconfirmed tx",
txBlockHeight: -1,
currentHeight: 100,
expectedConfirmations: 0,
- expectedHeight: 0,
+ expectedHeight: -1,
+ expectTimestamp: false,
},
{
name: "tx with 1 confirmation",
@@ -359,6 +364,7 @@ func TestGetTransactionConfirmations(t *testing.T) {
currentHeight: 100,
expectedConfirmations: 1,
expectedHeight: 100,
+ expectTimestamp: true,
},
{
name: "tx with 3 confirmations",
@@ -366,6 +372,7 @@ func TestGetTransactionConfirmations(t *testing.T) {
currentHeight: 10,
expectedConfirmations: 3,
expectedHeight: 8,
+ expectTimestamp: true,
},
{
name: "old tx with many confirmations",
@@ -373,6 +380,7 @@ func TestGetTransactionConfirmations(t *testing.T) {
currentHeight: 1000,
expectedConfirmations: 1000,
expectedHeight: 1,
+ expectTimestamp: true,
},
{
name: "tx in future block",
@@ -380,6 +388,7 @@ func TestGetTransactionConfirmations(t *testing.T) {
currentHeight: 100,
expectedConfirmations: 0,
expectedHeight: 105,
+ expectTimestamp: true,
},
}
@@ -447,6 +456,12 @@ func TestGetTransactionConfirmations(t *testing.T) {
require.Equal(t, tt.expectedHeight, result.Height)
+ if tt.expectTimestamp {
+ require.NotZero(t, result.Timestamp)
+ } else {
+ require.Zero(t, result.Timestamp)
+ }
+
// Additional checks for unconfirmed transactions.
if tt.txBlockHeight == -1 {
require.Nil(t, result.BlockHash)
@@ -459,6 +474,15 @@ func TestGetTransactionConfirmations(t *testing.T) {
require.Positive(
t, result.Confirmations,
)
+ } else {
+ // Confirmed txns in future blocks for
+ // example due to reorg should be
+ // treated as unconfirmed and have 0
+ // confirmations.
+ require.Equal(
+ t, int32(0),
+ result.Confirmations,
+ )
}
}
}) |
GustavoStingelin
left a comment
GustavoStingelin
left a comment
There was a problem hiding this comment.
Added two questions, but LGTM! 🔥
| switch { | ||
| case txHeight == -1, txHeight > curHeight: | ||
| // Unconfirmed transactions have 0 confirmations. | ||
| case txHeight == -1: |
There was a problem hiding this comment.
Q: just to exercise defensive programming, what happens if this function unforntly receives txHeight < -1?
There was a problem hiding this comment.
Q: just to exercise defensive programming, what happens if this function unforntly receives
txHeight < -1?
txHeight < -1 seems not possible, right? I am a bit hesitant if we put it here as defensive check we implicitly signal that it is possible that txHeight can be e.g -2 which is not the normal case
There was a problem hiding this comment.
Totally, in theory, this should not happen, since the function expects sanitized values. My concern is that designs that allow behavior that cannot be predicted tend to open space for future issues. This is not tied to this PR, but in my view it would be good to move away from this kind of pattern in the future, when we can.
3 participants
What is the issue?
GetTransaction()incorrectly sets theConfirmationsfield to the transaction's block height instead of calculating the actual number of confirmations.What is the underlying root cause behind that issue?
This is the main changeset 0b2e735 that introduced this bug as part of PR #890. The code directly assigns
Confirmations: txDetail.Block.Heightinstead of calculating confirmations ascurrentHeight - blockHeight + 1. For unconfirmed transactions whereBlock.Height == -1, this results in-1confirmations instead of the expected0.When was this issue introduced?
Active since: October 30, 2023 (module wallet/txauthor/v1.3.4).
Scope?
The bug is only isolated to
wallet.GetTransaction()publicly exposed API and any upstream software that uses it AND utilize theConfirmationsfield.Who is affected by that issue?
Since October 30, 2023: any software that uses this publicly exposed API
GetTransactionin btcwallet AND rely onConfirmationsfield for internal operations. Fortunately LND (verified releases <= 20.0) hasn't been impacted by the introduction of this bug (as yet) as well as Lightning Labs projects (verified loop, lightning-terminal, and taproot-assets). Initial dependency analysis shows major projects using btcwallet:How can someone reproduce that issue?
Seeing: 3da75f7
Failing Tests
How can we be sure and confident that this PR solves the root cause?
Running the tests after that commit 4263f16
Passing Tests
$ go test -v -run ^TestGetTransactionConfirmations$ github.com/btcsuite/btcwallet/wallet === RUN TestGetTransactionConfirmations === PAUSE TestGetTransactionConfirmations === CONT TestGetTransactionConfirmations === RUN TestGetTransactionConfirmations/unconfirmed_tx === PAUSE TestGetTransactionConfirmations/unconfirmed_tx === RUN TestGetTransactionConfirmations/tx_with_1_confirmation === PAUSE TestGetTransactionConfirmations/tx_with_1_confirmation === RUN TestGetTransactionConfirmations/tx_with_3_confirmations === PAUSE TestGetTransactionConfirmations/tx_with_3_confirmations === RUN TestGetTransactionConfirmations/tx_with_50_confirmations === PAUSE TestGetTransactionConfirmations/tx_with_50_confirmations === RUN TestGetTransactionConfirmations/old_tx_with_many_confirmations === PAUSE TestGetTransactionConfirmations/old_tx_with_many_confirmations === CONT TestGetTransactionConfirmations/unconfirmed_tx === CONT TestGetTransactionConfirmations/tx_with_50_confirmations === CONT TestGetTransactionConfirmations/tx_with_3_confirmations === CONT TestGetTransactionConfirmations/tx_with_1_confirmation === CONT TestGetTransactionConfirmations/old_tx_with_many_confirmations --- PASS: TestGetTransactionConfirmations (0.00s) --- PASS: TestGetTransactionConfirmations/unconfirmed_tx (6.46s) --- PASS: TestGetTransactionConfirmations/old_tx_with_many_confirmations (6.52s) --- PASS: TestGetTransactionConfirmations/tx_with_3_confirmations (6.60s) --- PASS: TestGetTransactionConfirmations/tx_with_50_confirmations (6.61s) --- PASS: TestGetTransactionConfirmations/tx_with_1_confirmation (6.63s) PASS ok github.com/btcsuite/btcwallet/wallet 6.648sPull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.