Auto merge of #3590 - bitcartel:fix_nullifier_persistence, r=bitcartel
Resolves Sapling nullifier persistence issue when importing a key. During a rescan, a CWalletTx was persisted to disk before it had its note data set. This meant that upon restart, the CWalletTx would potentially be missing its nullifiers causing the balance to include notes which had already been spent. The resolution is to force a CWalletTx to be persisted after it has had its nullifiers set correctly, before the note witnesses are updated.
This commit is contained in:
commit
2e0be155a2
|
@ -16,15 +16,16 @@ class WalletPersistenceTest (BitcoinTestFramework):
|
|||
|
||||
def setup_chain(self):
|
||||
print("Initializing test directory " + self.options.tmpdir)
|
||||
initialize_chain_clean(self.options.tmpdir, 2)
|
||||
initialize_chain_clean(self.options.tmpdir, 3)
|
||||
|
||||
def setup_network(self, split=False):
|
||||
self.nodes = start_nodes(2, self.options.tmpdir,
|
||||
self.nodes = start_nodes(3, self.options.tmpdir,
|
||||
extra_args=[[
|
||||
'-nuparams=5ba81b19:100', # Overwinter
|
||||
'-nuparams=76b809bb:201', # Sapling
|
||||
]] * 2)
|
||||
]] * 3)
|
||||
connect_nodes_bi(self.nodes,0,1)
|
||||
connect_nodes_bi(self.nodes,1,2)
|
||||
self.is_network_split=False
|
||||
self.sync_all()
|
||||
|
||||
|
@ -97,5 +98,34 @@ class WalletPersistenceTest (BitcoinTestFramework):
|
|||
assert_equal(self.nodes[0].z_getbalance(sapling_addr), Decimal('5'))
|
||||
assert_equal(self.nodes[1].z_getbalance(dest_addr), Decimal('15'))
|
||||
|
||||
# Verify importing a spending key will update and persist the nullifiers and witnesses correctly
|
||||
sk0 = self.nodes[0].z_exportkey(sapling_addr)
|
||||
self.nodes[2].z_importkey(sk0, "yes")
|
||||
assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('5'))
|
||||
|
||||
# Restart the nodes
|
||||
stop_nodes(self.nodes)
|
||||
wait_bitcoinds()
|
||||
self.setup_network()
|
||||
|
||||
# Verify nullifiers persisted correctly by checking balance
|
||||
# Prior to PR #3590, there will be an error as spent notes are considered unspent:
|
||||
# Assertion failed: expected: <25.00000000> but was: <5>
|
||||
assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('5'))
|
||||
|
||||
# Verity witnesses persisted correctly by sending shielded funds
|
||||
recipients = []
|
||||
recipients.append({"address": dest_addr, "amount": Decimal('1')})
|
||||
myopid = self.nodes[2].z_sendmany(sapling_addr, recipients, 1, 0)
|
||||
wait_and_assert_operationid_status(self.nodes[2], myopid)
|
||||
|
||||
self.sync_all()
|
||||
self.nodes[0].generate(1)
|
||||
self.sync_all()
|
||||
|
||||
# Verify balances
|
||||
assert_equal(self.nodes[2].z_getbalance(sapling_addr), Decimal('4'))
|
||||
assert_equal(self.nodes[1].z_getbalance(dest_addr), Decimal('16'))
|
||||
|
||||
if __name__ == '__main__':
|
||||
WalletPersistenceTest().main()
|
|
@ -2398,6 +2398,9 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
|
|||
const CChainParams& chainParams = Params();
|
||||
|
||||
CBlockIndex* pindex = pindexStart;
|
||||
|
||||
std::vector<uint256> myTxHashes;
|
||||
|
||||
{
|
||||
LOCK2(cs_main, cs_wallet);
|
||||
|
||||
|
@ -2418,9 +2421,11 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
|
|||
ReadBlockFromDisk(block, pindex);
|
||||
BOOST_FOREACH(CTransaction& tx, block.vtx)
|
||||
{
|
||||
if (AddToWalletIfInvolvingMe(tx, &block, fUpdate))
|
||||
if (AddToWalletIfInvolvingMe(tx, &block, fUpdate)) {
|
||||
myTxHashes.push_back(tx.GetHash());
|
||||
ret++;
|
||||
}
|
||||
}
|
||||
|
||||
SproutMerkleTree sproutTree;
|
||||
SaplingMerkleTree saplingTree;
|
||||
|
@ -2441,6 +2446,19 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
|
|||
LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex));
|
||||
}
|
||||
}
|
||||
|
||||
// After rescanning, persist Sapling note data that might have changed, e.g. nullifiers.
|
||||
// Do not flush the wallet here for performance reasons.
|
||||
CWalletDB walletdb(strWalletFile, "r+", false);
|
||||
for (auto hash : myTxHashes) {
|
||||
CWalletTx wtx = mapWallet[hash];
|
||||
if (!wtx.mapSaplingNoteData.empty()) {
|
||||
if (!wtx.WriteToDisk(&walletdb)) {
|
||||
LogPrintf("Rescanning... WriteToDisk failed to update Sapling note data for: %s\n", hash.ToString());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
ShowProgress(_("Rescanning..."), 100); // hide progress dialog in GUI
|
||||
}
|
||||
return ret;
|
||||
|
|
Loading…
Reference in New Issue