Skip to content

Commit 8aa2d31

Browse files
furszyfurszy
committed
Add block_height field in struct Confirmation
At wallet loading, we rely on chain state querying to retrieve height of txn, to do so we ensure that lock order is respected between cs_main and cs_wallet. If wallet loaded is the wallet-tool one, all wallet txn will show up with a height of zero. It doesn't matter as confirmation height is not used by wallet-tool. Reorder arguments and document Confirmation calls to avoid ambiguity. Adaptation of btc@5971d3848e09abf571e5308185275296127efca4
1 parent 4405ac0 commit 8aa2d31

File tree

6 files changed

+56
-30
lines changed

6 files changed

+56
-30
lines changed

‎src/test/librust/sapling_rpc_wallet_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling)
433433
chainActive.SetTip(&fakeIndex);
434434
BOOST_CHECK(chainActive.Contains(&fakeIndex));
435435
BOOST_CHECK_EQUAL(1, chainActive.Height());
436-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, blockHash, 0);
436+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, blockHash, 0);
437437
pwalletMain->LoadToWallet(wtx);
438438
BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed");
439439

‎src/test/librust/sapling_wallet_tests.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) {
232232
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
233233
BOOST_CHECK(saplingNoteData.size() > 0);
234234
wtx.SetSaplingNoteData(saplingNoteData);
235-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
235+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
236236
BOOST_CHECK(wallet.LoadToWallet(wtx));
237237

238238
// Simulate receiving new block and ChainTip signal
@@ -351,7 +351,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) {
351351
BOOST_CHECK(chainActive.Contains(&fakeIndex));
352352
BOOST_CHECK_EQUAL(0, chainActive.Height());
353353

354-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
354+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, 0, block.GetHash(), 0);
355355
wallet.LoadToWallet(wtx);
356356

357357
// Verify note has been spent
@@ -414,7 +414,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) {
414414
BOOST_CHECK_EQUAL(0, chainActive.Height());
415415

416416
// Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
417-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
417+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
418418
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
419419
BOOST_CHECK(saplingNoteData.size() > 0);
420420
wtx.SetSaplingNoteData(saplingNoteData);
@@ -512,7 +512,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
512512
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
513513
BOOST_CHECK(saplingNoteData.size() > 0);
514514
wtx.SetSaplingNoteData(saplingNoteData);
515-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
515+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
516516
wallet.LoadToWallet(wtx);
517517

518518
// Simulate receiving new block and ChainTip signal.
@@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
588588
auto saplingNoteData2 = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx2.tx).first;
589589
BOOST_CHECK(saplingNoteData2.size() > 0);
590590
wtx2.SetSaplingNoteData(saplingNoteData2);
591-
wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0);
591+
wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex2.nHeight, block2.GetHash(), 0);
592592
wallet.LoadToWallet(wtx2);
593593

594594
// Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers
@@ -967,7 +967,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) {
967967
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
968968
BOOST_CHECK(saplingNoteData.size() == 1); // wallet only has key for change output
969969
wtx.SetSaplingNoteData(saplingNoteData);
970-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
970+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
971971
wallet.LoadToWallet(wtx);
972972

973973
// Simulate receiving new block and ChainTip signal
@@ -1083,7 +1083,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) {
10831083
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
10841084
BOOST_CHECK(saplingNoteData.size() > 0);
10851085
wtx.SetSaplingNoteData(saplingNoteData);
1086-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
1086+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0);
10871087
wallet.LoadToWallet(wtx);
10881088

10891089
// Simulate receiving new block and ChainTip signal

‎src/wallet/test/wallet_shielded_balances_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree, CWallet
307307
chainActive.SetTip(fakeBlock.pindex);
308308
BOOST_CHECK(chainActive.Contains(fakeBlock.pindex));
309309
WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeBlock.pindex));
310-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0);
310+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->nHeight, fakeBlock.pindex->GetBlockHash(), 0);
311311
return fakeBlock;
312312
}
313313

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CWallet &wallet, CBlockIndex* pprev
333333
chainActive.SetTip(fakeIndex);
334334
BOOST_CHECK(chainActive.Contains(fakeIndex));
335335
WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeIndex));
336-
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0);
336+
wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex->nHeight, fakeIndex->GetBlockHash(), 0);
337337
removeTxFromMempool(wtx);
338338
wtx.fInMempool = false;
339339
return fakeIndex;

‎src/wallet/wallet.cpp‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -930,11 +930,13 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
930930
wtx.m_confirm.status = wtxIn.m_confirm.status;
931931
wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex;
932932
wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock;
933+
wtx.m_confirm.block_height = wtxIn.m_confirm.block_height;
933934
wtx.UpdateTimeSmart();
934935
fUpdated = true;
935936
} else {
936937
assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex);
937938
assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock);
939+
assert(wtx.m_confirm.block_height == wtxIn.m_confirm.block_height);
938940
}
939941
if (HasSaplingSPKM() && m_sspk_man->UpdatedNoteData(wtxIn, wtx)) {
940942
fUpdated = true;
@@ -971,17 +973,38 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
971973
return true;
972974
}
973975

976+
// Internal function for now, this will be part of a chain interface class in the future.
977+
Optional<int> getTipBlockHeight(const uint256& hash)
978+
{
979+
auto it = mapBlockIndex.find(hash);
980+
CBlockIndex* pindex = it == mapBlockIndex.end() ? nullptr : it->second;
981+
if (pindex && chainActive.Contains(pindex)) {
982+
return Optional<int>(pindex->nHeight);
983+
}
984+
return nullopt;
985+
}
986+
974987
bool CWallet::LoadToWallet(CWalletTx& wtxIn)
975988
{
976989
LOCK2(cs_main, cs_wallet);
977990
// If tx hasn't been reorged out of chain while wallet being shutdown
978991
// change tx status to UNCONFIRMED and reset hashBlock/nIndex.
979992
if (!wtxIn.m_confirm.hashBlock.IsNull()) {
980-
auto it = mapBlockIndex.find(wtxIn.m_confirm.hashBlock);
981-
CBlockIndex* pindex = it == mapBlockIndex.end() ? nullptr : it->second;
982-
if (!pindex || !chainActive.Contains(pindex)) {
993+
Optional<int> block_height = getTipBlockHeight(wtxIn.m_confirm.hashBlock);
994+
if (block_height) {
995+
// Update cached block height variable since it not stored in the
996+
// serialized transaction.
997+
wtxIn.m_confirm.block_height = *block_height;
998+
} else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) {
999+
// If tx block (or conflicting block) was reorged out of chain
1000+
// while the wallet was shutdown, change tx status to UNCONFIRMED
1001+
// and reset block height, hash, and index. ABANDONED tx don't have
1002+
// associated blocks and don't need to be updated. The case where a
1003+
// transaction was reorged out while online and then reconfirmed
1004+
// while offline is covered by the rescan logic.
9831005
wtxIn.setUnconfirmed();
9841006
wtxIn.m_confirm.hashBlock = UINT256_ZERO;
1007+
wtxIn.m_confirm.block_height = 0;
9851008
wtxIn.m_confirm.nIndex = 0;
9861009
}
9871010
}
@@ -997,7 +1020,7 @@ bool CWallet::LoadToWallet(CWalletTx& wtxIn)
9971020
if (it != mapWallet.end()) {
9981021
CWalletTx& prevtx = it->second;
9991022
if (prevtx.isConflicted()) {
1000-
MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash());
1023+
MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash());
10011024
}
10021025
}
10031026
}
@@ -1067,7 +1090,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CWallet
10671090
while (range.first != range.second) {
10681091
if (range.first->second != tx.GetHash()) {
10691092
LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), confirm.hashBlock.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n);
1070-
MarkConflicted(confirm.hashBlock, range.first->second);
1093+
MarkConflicted(confirm.hashBlock, confirm.block_height, range.first->second);
10711094
}
10721095
range.first++;
10731096
}
@@ -1152,7 +1175,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
11521175
if (currentconfirm == 0 && !wtx.isAbandoned()) {
11531176
// If the orig tx was not in block/mempool, none of its spends can be in mempool
11541177
assert(!wtx.InMempool());
1155-
wtx.m_confirm.nIndex = 0;
11561178
wtx.setAbandoned();
11571179
wtx.MarkDirty();
11581180
walletdb.WriteTx(wtx);
@@ -1179,7 +1201,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
11791201
return true;
11801202
}
11811203

1182-
void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
1204+
void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx)
11831205
{
11841206
LOCK2(cs_main, cs_wallet);
11851207

@@ -1219,6 +1241,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
12191241
// Mark transaction as conflicted with this block.
12201242
wtx.m_confirm.nIndex = 0;
12211243
wtx.m_confirm.hashBlock = hashBlock;
1244+
wtx.m_confirm.block_height = conflicting_height;
12221245
wtx.setConflicted();
12231246
wtx.MarkDirty();
12241247
walletdb.WriteTx(wtx);
@@ -1254,7 +1277,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CWalletTx::Confi
12541277
void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx)
12551278
{
12561279
LOCK2(cs_main, cs_wallet);
1257-
CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0);
1280+
CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
12581281
SyncTransaction(ptx, confirm);
12591282

12601283
auto it = mapWallet.find(ptx->GetHash());
@@ -1278,10 +1301,10 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
12781301
m_last_block_processed = pindex->GetBlockHash();
12791302
m_last_block_processed_time = pindex->GetBlockTime();
12801303
m_last_block_processed_height = pindex->nHeight;
1281-
for (size_t i = 0; i < pblock->vtx.size(); i++) {
1282-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed->GetBlockHash(), i);
1283-
SyncTransaction(pblock->vtx[i], confirm);
1284-
TransactionRemovedFromMempool(pblock->vtx[i]);
1304+
for (size_t index = 0; index < pblock->vtx.size(); index++) {
1305+
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed_height, m_last_block_processed->GetBlockHash(), index);
1306+
SyncTransaction(pblock->vtx[index], confirm);
1307+
TransactionRemovedFromMempool(pblock->vtx[index]);
12851308
}
12861309
for (const CTransactionRef& ptx : vtxConflicted) {
12871310
TransactionRemovedFromMempool(ptx);
@@ -1316,7 +1339,7 @@ void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, con
13161339
m_last_block_processed_time = blockTime;
13171340
m_last_block_processed = blockHash;
13181341
for (const CTransactionRef& ptx : pblock->vtx) {
1319-
CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0);
1342+
CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
13201343
SyncTransaction(ptx, confirm);
13211344
}
13221345

@@ -1848,7 +1871,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b
18481871
}
18491872
for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) {
18501873
const auto& tx = block.vtx[posInBlock];
1851-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock);
1874+
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, pindex->nHeight, pindex->GetBlockHash(), posInBlock);
18521875
if (AddToWalletIfInvolvingMe(tx, confirm, fUpdate)) {
18531876
myTxHashes.push_back(tx->GetHash());
18541877
ret++;

‎src/wallet/wallet.h‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -364,15 +364,17 @@ class CWalletTx
364364
ABANDONED
365365
};
366366

367-
/* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed.
368-
* This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state
369-
* where they instead point to block hash and index of the deepest conflicting tx.
367+
/* Confirmation includes tx status and a triplet of {block height/block hash/tx index in block}
368+
* at which tx has been confirmed. All three are set to 0 if tx is unconfirmed or abandoned.
369+
* Meaning of these fields changes with CONFLICTED state where they instead point to block hash
370+
* and block height of the deepest conflicting tx.
370371
*/
371372
struct Confirmation {
372373
Status status;
374+
int block_height;
373375
uint256 hashBlock;
374376
int nIndex;
375-
Confirmation(Status s = UNCONFIRMED, uint256 h = uint256(), int i = 0) : status(s), hashBlock(h), nIndex(i) {}
377+
Confirmation(Status s = UNCONFIRMED, int b = 0, const uint256& h = UINT256_ZERO, int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {}
376378
};
377379

378380
Confirmation m_confirm;
@@ -423,7 +425,6 @@ class CWalletTx
423425
* compatibility (pre-commit 9ac63d6).
424426
*/
425427
if (serializedIndex == -1 && m_confirm.hashBlock == ABANDON_HASH) {
426-
m_confirm.hashBlock = uint256();
427428
setAbandoned();
428429
} else if (serializedIndex == -1) {
429430
setConflicted();
@@ -528,12 +529,14 @@ class CWalletTx
528529
{
529530
m_confirm.status = CWalletTx::ABANDONED;
530531
m_confirm.hashBlock = UINT256_ZERO;
532+
m_confirm.block_height = 0;
531533
m_confirm.nIndex = 0;
532534
}
533535
bool isConflicted() const { return m_confirm.status == CWalletTx::CONFLICTED; }
534536
void setConflicted() { m_confirm.status = CWalletTx::CONFLICTED; }
535537
bool isUnconfirmed() const { return m_confirm.status == CWalletTx::UNCONFIRMED; }
536538
void setUnconfirmed() { m_confirm.status = CWalletTx::UNCONFIRMED; }
539+
bool isConfirmed() const { return m_confirm.status == CWalletTx::CONFIRMED; }
537540
void setConfirmed() { m_confirm.status = CWalletTx::CONFIRMED; }
538541

539542
const uint256& GetHash() const { return tx->GetHash(); }
@@ -602,7 +605,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
602605
void AddToSpends(const uint256& wtxid);
603606

604607
/* Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */
605-
void MarkConflicted(const uint256& hashBlock, const uint256& hashTx);
608+
void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx);
606609

607610
template <class T>
608611
void SyncMetaData(std::pair<typename TxSpendMap<T>::iterator, typename TxSpendMap<T>::iterator> range);

0 commit comments

Comments
 (0)