Skip to content

Commit

Permalink
Encapsulate tx status in a Confirmation struct
Browse files Browse the repository at this point in the history 
Adaptation of btc@a31be09bfd77eed497a8e251d31358e16e2f2eb1

Instead of relying on combination of hashBlock and nIndex
values to manage tx in its lifecycle, we introduce 4
status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED.

hashBlock and nIndex magic values should only be used at
serialization/deserialization for backward-compatibility.

At block disconnection, we know flag txn as UNCONFIRMED where
previously they kept their states until being override by a
block connection or abandontransaction call. This is a change
in behavior for which user may have to call abandon twice
if transaction is disconnected and not accepted back in the mempool.

We assert status transitioning right in AddToWallet. Doing so
flagged a misbehavior in ComputeTimeSmart unit test where same
tx is confirmed twice in different block. To avoid inconsistencies
we unconfirmed tx before new connection in different block. We
also remove a cs_main lock in test, as AddToWallet and its
callees don't rely on locked chain.
  • Loading branch information
@furszy
Antoine Riard authored and furszy committed Mar 10, 2021
1 parent 6438931 commit 8d8928e
Show file tree
Hide file tree
Showing 8 changed files with 129 additions and 76 deletions.
2 changes: 1 addition & 1 deletion src/qt/transactionrecord.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ void TransactionRecord::updateStatus(const CWalletTx& wtx)

CBlockIndex *pindex = nullptr;
// Find the block the tx is in
BlockMap::iterator mi = mapBlockIndex.find(wtx.hashBlock);
BlockMap::iterator mi = mapBlockIndex.find(wtx.m_confirm.hashBlock);
if (mi != mapBlockIndex.end())
pindex = (*mi).second;

Expand Down
2 changes: 1 addition & 1 deletion src/test/librust/sapling_rpc_wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling)
chainActive.SetTip(&fakeIndex);
BOOST_CHECK(chainActive.Contains(&fakeIndex));
BOOST_CHECK_EQUAL(1, chainActive.Height());
wtx.SetMerkleBranch(blockHash, 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, blockHash, 0);
pwalletMain->LoadToWallet(wtx);
BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed");

Expand Down
14 changes: 7 additions & 7 deletions src/test/librust/sapling_wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) {
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
BOOST_CHECK(saplingNoteData.size() > 0);
wtx.SetSaplingNoteData(saplingNoteData);
wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
BOOST_CHECK(wallet.LoadToWallet(wtx));

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

wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
wallet.LoadToWallet(wtx);

// Verify note has been spent
Expand Down Expand Up @@ -414,7 +414,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) {
BOOST_CHECK_EQUAL(0, chainActive.Height());

// Simulate SyncTransaction which calls AddToWalletIfInvolvingMe
wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
BOOST_CHECK(saplingNoteData.size() > 0);
wtx.SetSaplingNoteData(saplingNoteData);
Expand Down Expand Up @@ -512,7 +512,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
BOOST_CHECK(saplingNoteData.size() > 0);
wtx.SetSaplingNoteData(saplingNoteData);
wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
wallet.LoadToWallet(wtx);

// Simulate receiving new block and ChainTip signal.
Expand Down Expand Up @@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
auto saplingNoteData2 = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx2.tx).first;
BOOST_CHECK(saplingNoteData2.size() > 0);
wtx2.SetSaplingNoteData(saplingNoteData2);
wtx2.SetMerkleBranch(block2.GetHash(), 0);
wtx2.SetConf(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0);
wallet.LoadToWallet(wtx2);

// Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers
Expand Down Expand Up @@ -967,7 +967,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) {
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
BOOST_CHECK(saplingNoteData.size() == 1); // wallet only has key for change output
wtx.SetSaplingNoteData(saplingNoteData);
wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
wallet.LoadToWallet(wtx);

// Simulate receiving new block and ChainTip signal
Expand Down Expand Up @@ -1083,7 +1083,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) {
auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first;
BOOST_CHECK(saplingNoteData.size() > 0);
wtx.SetSaplingNoteData(saplingNoteData);
wtx.SetMerkleBranch(block.GetHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0);
wallet.LoadToWallet(wtx);

// Simulate receiving new block and ChainTip signal
Expand Down
12 changes: 6 additions & 6 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ void WalletTxToJSON(const CWalletTx& wtx, UniValue& entry)
if (wtx.IsCoinBase() || wtx.IsCoinStake())
entry.pushKV("generated", true);
if (confirms > 0) {
entry.pushKV("blockhash", wtx.hashBlock.GetHex());
entry.pushKV("blockindex", wtx.nIndex);
entry.pushKV("blocktime", mapBlockIndex[wtx.hashBlock]->GetBlockTime());
entry.pushKV("blockhash", wtx.m_confirm.hashBlock.GetHex());
entry.pushKV("blockindex", wtx.m_confirm.nIndex);
entry.pushKV("blocktime", mapBlockIndex[wtx.m_confirm.hashBlock]->GetBlockTime());
} else {
entry.pushKV("trusted", wtx.IsTrusted());
}
Expand Down Expand Up @@ -2563,9 +2563,9 @@ UniValue listreceivedbyshieldaddress(const JSONRPCRequest& request)

if (pwalletMain->mapWallet.count(entry.op.hash)) {
const CWalletTx& wtx = pwalletMain->mapWallet.at(entry.op.hash);
if (!wtx.hashBlock.IsNull())
height = mapBlockIndex[wtx.hashBlock]->nHeight;
index = wtx.nIndex;
if (!wtx.m_confirm.hashBlock.IsNull())
height = mapBlockIndex[wtx.m_confirm.hashBlock]->nHeight;
index = wtx.m_confirm.nIndex;
time = wtx.GetTxTime();
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/wallet_shielded_balances_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree)
fakeBlock.pindex->phashBlock = &mapBlockIndex.find(fakeBlock.block.GetHash())->first;
chainActive.SetTip(fakeBlock.pindex);
BOOST_CHECK(chainActive.Contains(fakeBlock.pindex));
wtx.SetMerkleBranch(fakeBlock.pindex->GetBlockHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0);
return fakeBlock;
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr)
fakeIndex->phashBlock = &mapBlockIndex.find(block.GetHash())->first;
chainActive.SetTip(fakeIndex);
BOOST_CHECK(chainActive.Contains(fakeIndex));
wtx.SetMerkleBranch(fakeIndex->GetBlockHash(), 0);
wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0);
removeTxFromMempool(wtx);
wtx.fInMempool = false;
return fakeIndex;
Expand Down
89 changes: 45 additions & 44 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,21 +926,15 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)

bool fUpdated = false;
if (!fInsertedNew) {
// Merge
if (!wtxIn.hashUnset() && wtxIn.hashBlock != wtx.hashBlock) {
wtx.hashBlock = wtxIn.hashBlock;
if (wtxIn.m_confirm.status != wtx.m_confirm.status) {
wtx.m_confirm.status = wtxIn.m_confirm.status;
wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex;
wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock;
wtx.UpdateTimeSmart();
fUpdated = true;
}
// If no longer abandoned, update
if (wtxIn.hashBlock.IsNull() && wtx.isAbandoned()) {
wtx.hashBlock = wtxIn.hashBlock;
if (!fUpdated) wtx.UpdateTimeSmart();
fUpdated = true;
}
if (wtxIn.nIndex != -1 && wtxIn.nIndex != wtx.nIndex) {
wtx.nIndex = wtxIn.nIndex;
fUpdated = true;
} else {
assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex);
assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock);
}
if (HasSaplingSPKM() && m_sspk_man->UpdatedNoteData(wtxIn, wtx)) {
fUpdated = true;
Expand Down Expand Up @@ -990,8 +984,8 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
auto it = mapWallet.find(txin.prevout.hash);
if (it != mapWallet.end()) {
CWalletTx& prevtx = it->second;
if (prevtx.nIndex == -1 && !prevtx.hashUnset()) {
MarkConflicted(prevtx.hashBlock, wtx.GetHash());
if (prevtx.isConflicted()) {
MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash());
}
}
}
Expand Down Expand Up @@ -1049,7 +1043,7 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const
* Abandoned state should probably be more carefully tracked via different
* posInBlock signals or by checking mempool presence when necessary.
*/
bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256& blockHash, int posInBlock, bool fUpdate)
bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate)
{
const CTransaction& tx = *ptx;
{
Expand Down Expand Up @@ -1103,10 +1097,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
if (isFromMe) AddExternalNotesDataToTx(wtx);
}

// Get merkle branch if transaction was found in a block
if (!blockHash.IsNull()) {
wtx.SetMerkleBranch(blockHash, posInBlock);
}
// Block disconnection override an abandoned tx as unconfirmed
// which means user may have to call abandontransaction again
wtx.SetConf(status, blockHash, posInBlock);

return AddToWallet(wtx, false);
}
Expand Down Expand Up @@ -1147,7 +1140,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
if (currentconfirm == 0 && !wtx.isAbandoned()) {
// If the orig tx was not in block/mempool, none of its spends can be in mempool
assert(!wtx.InMempool());
wtx.nIndex = -1;
wtx.m_confirm.nIndex = 0;
wtx.setAbandoned();
wtx.MarkDirty();
walletdb.WriteTx(wtx);
Expand Down Expand Up @@ -1212,8 +1205,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
if (conflictconfirms < currentconfirm) {
// Block is 'more conflicted' than current confirm; update.
// Mark transaction as conflicted with this block.
wtx.nIndex = -1;
wtx.hashBlock = hashBlock;
wtx.m_confirm.nIndex = 0;
wtx.m_confirm.hashBlock = hashBlock;
wtx.setConflicted();
wtx.MarkDirty();
walletdb.WriteTx(wtx);
// Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too
Expand All @@ -1236,9 +1230,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
}
}

void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock)
void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status status, const CBlockIndex *pindexBlockConnected, int posInBlock)
{
if (!AddToWalletIfInvolvingMe(ptx,
status,
(pindexBlockConnected) ? pindexBlockConnected->GetBlockHash() : uint256(),
posInBlock,
true)) {
Expand All @@ -1251,7 +1246,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pin
void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx)
{
LOCK2(cs_main, cs_wallet);
SyncTransaction(ptx, NULL, -1);
SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1);

auto it = mapWallet.find(ptx->GetHash());
if (it != mapWallet.end()) {
Expand Down Expand Up @@ -1279,11 +1274,11 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
// the notification that the conflicted transaction was evicted.

for (const CTransactionRef& ptx : vtxConflicted) {
SyncTransaction(ptx, nullptr, -1);
SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, nullptr, -1);
TransactionRemovedFromMempool(ptx);
}
for (size_t i = 0; i < pblock->vtx.size(); i++) {
SyncTransaction(pblock->vtx[i], pindex, i);
SyncTransaction(pblock->vtx[i], CWalletTx::Status::CONFIRMED, pindex, i);
TransactionRemovedFromMempool(pblock->vtx[i]);
}

Expand All @@ -1309,8 +1304,13 @@ void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const
void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, int nBlockHeight)
{
LOCK2(cs_main, cs_wallet);

// At block disconnection, this will change an abandoned transaction to
// be unconfirmed, whether or not the transaction is added back to the mempool.
// User may have to call abandontransaction again. It may be addressed in the
// future with a stickier abandoned state or even removing abandontransaction call.
for (const CTransactionRef& ptx : pblock->vtx) {
SyncTransaction(ptx, NULL, -1);
SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1);
}

if (Params().GetConsensus().NetworkUpgradeActive(nBlockHeight, Consensus::UPGRADE_V5_0)) {
Expand Down Expand Up @@ -1476,11 +1476,11 @@ int64_t CWalletTx::GetTxTime() const
void CWalletTx::UpdateTimeSmart()
{
nTimeSmart = nTimeReceived;
if (!hashBlock.IsNull()) {
if (mapBlockIndex.count(hashBlock)) {
nTimeSmart = mapBlockIndex.at(hashBlock)->GetBlockTime();
if (!m_confirm.hashBlock.IsNull()) {
if (mapBlockIndex.count(m_confirm.hashBlock)) {
nTimeSmart = mapBlockIndex.at(m_confirm.hashBlock)->GetBlockTime();
} else
LogPrintf("%s : found %s in block %s not in index\n", __func__, GetHash().ToString(), hashBlock.ToString());
LogPrintf("%s : found %s in block %s not in index\n", __func__, GetHash().ToString(), m_confirm.hashBlock.ToString());
}
}

Expand Down Expand Up @@ -1837,7 +1837,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b
}
for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) {
const auto& tx = block.vtx[posInBlock];
if (AddToWalletIfInvolvingMe(tx, pindex->GetBlockHash(), posInBlock, fUpdate)) {
if (AddToWalletIfInvolvingMe(tx, CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock, fUpdate)) {
myTxHashes.push_back(tx->GetHash());
ret++;
}
Expand Down Expand Up @@ -3749,7 +3749,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const
for (std::map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); it++) {
// iterate over all wallet transactions...
const CWalletTx& wtx = (*it).second;
BlockMap::const_iterator blit = mapBlockIndex.find(wtx.hashBlock);
BlockMap::const_iterator blit = mapBlockIndex.find(wtx.m_confirm.hashBlock);
if (blit != mapBlockIndex.end() && chainActive.Contains(blit->second)) {
// ... which are already in a block
int nHeight = blit->second->nHeight;
Expand Down Expand Up @@ -4283,13 +4283,16 @@ CWalletKey::CWalletKey(int64_t nExpires)
nTimeExpires = nExpires;
}

void CWalletTx::SetMerkleBranch(const uint256& blockHash, int posInBlock)
void CWalletTx::SetConf(Status status, const uint256& blockHash, int posInBlock)
{
// Update tx status
m_confirm.status = status;

// Update the tx's hashBlock
hashBlock = blockHash;
m_confirm.hashBlock = blockHash;

// set the position of the transaction in the block
nIndex = posInBlock;
m_confirm.nIndex = posInBlock;
}

int CWalletTx::GetDepthInMainChain() const
Expand All @@ -4300,13 +4303,12 @@ int CWalletTx::GetDepthInMainChain() const

int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const
{
if (hashUnset())
return 0;
if (isUnconfirmed() || isAbandoned()) return 0;
AssertLockHeld(cs_main);
int nResult;

// Find the block it claims to be in
BlockMap::iterator mi = mapBlockIndex.find(hashBlock);
BlockMap::iterator mi = mapBlockIndex.find(m_confirm.hashBlock);
if (mi == mapBlockIndex.end()) {
nResult = 0;
} else {
Expand All @@ -4315,7 +4317,7 @@ int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const
nResult = 0;
} else {
pindexRet = pindex;
nResult = ((nIndex == -1) ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1);
nResult = (isConflicted() ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1);
}
}

Expand Down Expand Up @@ -4604,9 +4606,7 @@ bool CWallet::LoadSaplingPaymentAddress(
////////////////////////////////////////////////////////////

CWalletTx::CWalletTx(const CWallet* pwalletIn, CTransactionRef arg)
: tx(std::move(arg)),
hashBlock(uint256()),
nIndex(-1)
: tx(std::move(arg))
{
Init(pwalletIn);
}
Expand All @@ -4628,6 +4628,7 @@ void CWalletTx::Init(const CWallet* pwalletIn)
fShieldedChangeCached = false;
nShieldedChangeCached = 0;
nOrderPos = -1;
m_confirm = Confirmation{};
}

bool CWalletTx::IsTrusted() const
Expand Down
Loading

0 comments on commit 8d8928e

Please sign in to comment.