Drop checkFinalTx and use Median Time Past to check finality of wallet transactions #17443

ariard · 2019-11-11T18:37:52Z

Replace Chain::checkFinalTx by CheckFinalWalletTx by caching best block median time past which is following BIP113 rule than time-locked transactions have nLockTime set to less than the median time of the previous block they're contained in.

Fix misuage of CheckFinalTx, which was called with default flags
argument by implementation of Chain::checkFinal, triggering
finality evaluation based on GetAdjustedTime instead of consensus
and standard rules of BIP 113.

I think it should have been set at same time than d1c3762, to align wallet checks on the mempool ones.

jonatack

Apologies for beginning with nits rather than concept review as I lack the time today to review this properly. A few grammar suggestions for clarity, if the fundamental changes are correct.

src/validation.cpp

DrahtBot · 2019-11-11T19:38:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: replace pointers by references within tx_verify.{h,cpp} #19069 (refactor: replace pointers by references within tx_verify.{h,cpp} by theStack)
doc: IsFinalTx comment about nSequence & OP_CLTV #18096 (doc: IsFinalTx comment about nSequence & OP_CLTV by nothingmuch)
wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)
Wallet passive startup #15719 (Wallet passive startup by ryanofsky)
[RPC] Include coinbase transactions in receivedby RPCs #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky

Code review ACK c9609737208a238984717d16e13f770ea350bb95. This seems like a good change because it should make checkFinalTx more accurate given current consensus rules, and also because it should make IsTrusted more conservative. Two suggestions for followup here or a later PR:

It could be good to add a new test for CWallet::IsTrusted setting a mock time and covering this edge case. I think this should not be too hard to implement as either a c++ or a python test
Especially with #16426, I wonder if it would be better to delete the interfaces::Chain::checkFinalTx method, and instead add a CWallet::CheckFinalTx method calling IsFinalTx with m_last_block_processed height and time, so the wallet uses a consistent tip and there aren't race conditions when the tip is changing. I previously suggested this in #16426 (comment)). This could also be added as another Chain interface TODO item if it is not worth the effort to implement now.

src/validation.cpp

ariard · 2019-12-18T00:49:41Z

@ryanofsky Thanks for review!

I went ahead on replaced checkFinalTx by CheckFinalWalletTx which itself calls IsFinalTx by caching median time past in the wallet.

About testing, I think a bit about it, it's quite hard because contrary to the consensus code where finality enforcement is going to change after softfork activation, wallet is always using current finality consensus rules. So that would need to have 2 versions of the wallet. I think that's an issue already known of our current wallet test framework..

ariard · 2019-12-18T18:28:25Z

01377a5

Moved IsFinalTx from consensus/tx_verify.h to consensus/tx_check.h as it's context-independent and so let wallet code called it without linking against server one. Should fix travis build failure.

ryanofsky

Code review ACK 01377a55545beb03c6c0033f4bea483338855880 modulo some new travis failures in miner_tests.cpp. Various suggested changes since last review (thanks!)

src/validation.cpp

src/wallet/wallet.cpp

ariard

Thanks for review @ryanofsky! Updated at 6fb938c with fixing travis build failure.

ryanofsky

Code review ACK 6fb938c78d1fed9e460775f772e276b473075965. No change since last review other than fixing travis include error. There is still a remaining travis error though:

https://travis-ci.org/bitcoin/bitcoin/jobs/626988480#L3179

wallet/wallet.cpp:3999:26: error: reading variable 'm_last_block_processed_height' requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-analysis]
    return IsFinalTx(tx, m_last_block_processed_height + 1, m_last_block_median_time_past);

ariard · 2020-01-03T23:32:49Z

Updated bbdc8b5, extended both IsTrusted declarations with NO_THREAD_SAFETY_ANALYSIS to pass static analysis on macOS and so remove travis failure.

ryanofsky

Conditional code review ACK fa5b990647baf7184d1fd610f9a747be3ed46127 if the NO_THREAD_SAFETY_ANALYSIS annotations can be removed, or at least documented (see comment below). Only changes since the last review are adding the annotations and fixing up some comments in the third commit message.

ryanofsky · 2020-01-08T22:28:26Z

src/wallet/wallet.h

In commit "Replace Chain::checkFinal by CWallet::CheckFinalWalletTx" (1f479751b2eaf6f17ae76334b72ebc5fd76d8956)

I think it'd be safer to just add AssertLockHeld statements if the compiler can't determine that a particular lock is held, than to disable safety analysis altogether. On my system, the following changes seem to work:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9d291f0b585..c702f90681d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1882,6 +1882,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const { // Quick answer in most cases + AssertLockHeld(pwallet->cs_wallet); if (!pwallet->CheckFinalWalletTx(*tx)) return false; int nDepth = GetDepthInMainChain(); if (nDepth >= 1) return true; @@ -3967,6 +3968,7 @@ bool CWalletTx::IsImmatureCoinBase() const bool CWalletTx::isFinal() const { + AssertLockHeld(pwallet->cs_wallet); return pwallet->CheckFinalWalletTx(*tx); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index acf0de45f1c..4ae39ba328a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -477,8 +477,8 @@ public: bool IsEquivalentTo(const CWalletTx& tx) const; bool InMempool() const; - bool IsTrusted(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS; - bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const NO_THREAD_SAFETY_ANALYSIS; + bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; + bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const; int64_t GetTxTime() const; @@ -531,7 +531,7 @@ public: const uint256& GetHash() const { return tx->GetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsImmatureCoinBase() const; - bool isFinal() const NO_THREAD_SAFETY_ANALYSIS; + bool isFinal() const; }; class COutput

6e10c40e9507c2ff45d47ca863778fff85584b36

Did you test on a Linux ? IIRC clang static analysis only work on MacOS. AssertLockHeld is lock checking at runtime

Did you test on a Linux ? IIRC clang static analysis only work on MacOS. AssertLockHeld is lock checking at runtime

AssertLockHeld does affect static analysis, because it sets the assert_exclusive_lock attribute, see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html. I am testing on linux, and I am using clang and have static analysis enabled. The following change works fine for me with the asserts added, so still think NO_THREAD_SAFETY_ANALYSIS annotations are unneeded.

--- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -481,12 +481,12 @@ public: // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid // having to resolve the issue of member access into incomplete type CWallet. - bool IsTrusted(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS; + bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid // having to resolve the issue of member access into incomplete type CWallet. - bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const NO_THREAD_SAFETY_ANALYSIS; + bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const; int64_t GetTxTime() const; @@ -543,7 +543,7 @@ public: // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid // having to resolve the issue of member access into incomplete type CWallet. - bool isFinal() const NO_THREAD_SAFETY_ANALYSIS; + bool isFinal() const; };

ariard

Updated at 9afb8dd, thanks to your suggestions @ryanofsky, AssertLockHeld being disabled with default flags it's not going to be a performance hit, but will hit deadlock in Travis.

ryanofsky

Code review ACK 9afb8ddd1d9406a623d209bc5a4a5e955f0b806c. Only change since last review is to lock assertions, annotations, and comments

fjahr

ACK 9afb8ddd1d9406a623d209bc5a4a5e955f0b806c

Reviewed code and ran tests locally. I think it's a good change both as an incremental step for wallet/node separation and for the fix on CheckFinalTx usage.

Just some grammar nits in a comment block. And, if you get around to it, I also saw two typos in the commit messages:
70cc88fa8bfcb3032ee8d63120921b8448b10e8e => mediam
f50723670e104457c21ca8b214e0b46bf6d23df2 => misuage

src/wallet/wallet.cpp

ryanofsky

Code review ACK b14e07b227f3830120e97f44b3d6ece55bf700c6. Just rebased since previous review

src/wallet/wallet.cpp

jonatack

Concept ACK / almost ACK b14e07b227f3830120e9 but this seems unfinished. The diff isn't large; best to not add TODOs and debt for later that can be done here, tie off the loose ends like #17443 (comment), and take care in the writing of the code docs and commit messages. Could use a test to be confident it's not adding a regression.

jonatack · 2020-05-15T08:04:57Z

src/consensus/tx_check.cpp

a20187d suggestion while moving this code

-bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) +bool IsFinalTx(const CTransaction& tx, int nBlockHeight, int64_t nBlockTime)

src/wallet/wallet.cpp

src/wallet/wallet.h

src/wallet/wallet.cpp

jonatack · 2020-05-15T09:13:37Z

src/wallet/rpcwallet.cpp

Nice improvement here!

ariard · 2020-05-19T08:16:49Z

Thanks @ryanofsky and @jonatack for reviews, took most of your suggestion, specially with better comment for top message commit.

ryanofsky

Code review ACK 808dc174b14fb7504c74a818ecaa2399258cf94e. Changes since last review: updating findBlock calls mostly as suggested (still not raising error on failure), updating commit messages, comments, thread annotations

ryanofsky · 2020-05-19T12:05:30Z

src/wallet/wallet.h

In commit "[wallet] Replace Chain::checkFinal by CWallet::CheckFinalWalletTx" (03870881969b12d8023716897ff674ec6ec3568e)

Travis error here:

In file included from interfaces/wallet.cpp:24: ./wallet/wallet.h:558:51: error: use of undeclared identifier 'cs_wallet'; did you mean 'pwallet'? bool isFinal() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); ^~~~~~~~~ pwallet ./threadsafety.h:31:79: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED' #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) ^ ./wallet/wallet.h:272:20: note: 'pwallet' declared here const CWallet* pwallet;

https://travis-ci.org/github/bitcoin/bitcoin/jobs/688716547#L3380

Probably needs to say pwallet->cs_wallet not just cs_wallet

I wonder @ariard if you're able to test lock annotations locally since these errors seem to come up a lot. It shouldn't be hard to enable them. I just configure with ./configure CXX=clang++ --enable-debug --enable-werror for these

Seeing the same messages while building locally, the gcc 8.3 build is fine but the clang 6.0 debug/werror build errors.

Building with pwallet->cs_wallet halts with

./wallet/wallet.h:558:58: error: member access into incomplete type 'const CWallet' bool isFinal() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet); ^ ./wallet/fees.h:13:7: note: forward declaration of 'CWallet' class CWallet; ^

@jonatack @ryanofsky You right I didn't the warning at first, was compiling with gcc locally. Okay in fact the issue hasn't been solved wrt to NO_THREAD_SAFETY_ANALYSIS.

Isn't the way to solve it cleanly would be to get CWalletTx its own lock instead of requiring CWallet one ? I don't see a better way to solve dependency declaration..

Updated dropping isFinal accessor on the advice of @ryanofsky

jonatack · 2020-05-19T12:51:43Z

re-almost ACK 808dc17 the changes per git diff b14e07b 808dc17 look good (thanks!); building locally with Clang is raising as mentioned above.

fjahr · 2020-05-23T12:12:55Z

I also see the build error locally (I use clang by default). Code looks good but will wait with a full review until the fix is up.

Context-independent IsFinalTx doesn't rely on chain or mempool state so moving it to tx_check let it being used by wallet code without linking server code in following commits.

Extend Notifications::BlockConnected/BlockDisconnected with median_time_past parameter. Used in next commit.

By caching tip median time-past, wallet can do transactions finality calculation without accessing chain state. Fix misuage of CheckFinalTx, which was called with default flags argument by implementation of Chain::checkFinal, triggering finality evaluation based on GetAdjustedTime instead of consensus and standard rules of BIP 113.

nBlockTime semantic is selected using a bit-flags corresponding to current soft-fork rules. Comment around this bit-flags was previously ambiguous meaning that not soft-fork rules affecting nBlockTime was currently active, which is false since BIP113 deployement.

ariard · 2020-06-04T02:07:14Z

Rebased and updated with fix at d55ba31. Thanks for your patience!

DrahtBot · 2020-06-08T16:34:30Z

🐙 This pull request conflicts with the target branch and needs rebase.

ryanofsky

Code review ACK d55ba31. No significant changes since last review, just replaced wtx.isFinal calls with wallet.CheckFinalWalletTx to work around thread annotation problems

ryanofsky · 2020-06-12T02:25:23Z

src/wallet/wallet.cpp

    if (tip_height) {
        walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
        walletInstance->m_last_block_processed_height = *tip_height;
+        chain.findBlock(walletInstance->m_last_block_processed, FoundBlock().mtpTime(walletInstance->m_last_block_median_time_past));


In commit "[wallet] Add m_last_block_mediam_time_past in CWallet" (1d2345b)

Suggest using CHECK_NONFATAL(chain.findBlock...) to catch a failure early if chain.findBlock returns not found for some reason

DrahtBot · 2021-12-15T11:22:21Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2022-03-21T13:07:45Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2022-03-21T13:22:33Z

I think the third commit is no longer relevant, because this was fixed/removed in commit e30b6ea.
The forth commit is no longer relevant, because this was removed in commit 28bdaa3.

maflcko · 2022-03-21T13:24:16Z

If there is anything relevant that I missed, I think it might be best to open a fresh pull request.

ariard · 2022-04-03T19:07:36Z

I agree this PR is not relevant anymore in its substantial parts.
I've looked on e30b6ea, effectively we don't need anymore checkFinalTx as we rely directly on CWalletTx::InMempool() in AvailableCoins() and CachedTxIsTrusted() to filter out the non-final transactions.
Also 28bdaa3 sounds good to me, as we only evaluate transaction finality at tip, at which BIP 113 is always active.
The first commit, moving IsFinalTx from tx_verify.h to tx_check.h is not relevant anymore as there is no more non-server caller of the moved function.

jonatack reviewed Nov 11, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot added the Validation label Nov 11, 2019

jnewbery added Wallet and removed Validation labels Nov 17, 2019

ryanofsky approved these changes Dec 16, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Dec 16, 2019

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

ariard force-pushed the 2019-11-fix-final-wallet branch from c960973 to 3af0df2 Compare December 18, 2019 00:36

ariard changed the title ~~Use Median Time Past to check finality of wallet transactions~~ Drop checkFinalTx and use Median Time Past to check finality of wallet transactions Dec 18, 2019

ariard force-pushed the 2019-11-fix-final-wallet branch from 3af0df2 to 01377a5 Compare December 18, 2019 18:27

ryanofsky approved these changes Dec 18, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ariard force-pushed the 2019-11-fix-final-wallet branch from 01377a5 to 6fb938c Compare December 19, 2019 01:08

ariard commented Dec 19, 2019

View reviewed changes

ryanofsky approved these changes Dec 19, 2019

View reviewed changes

ariard force-pushed the 2019-11-fix-final-wallet branch from 6fb938c to 6e10c40 Compare December 19, 2019 17:45

ariard force-pushed the 2019-11-fix-final-wallet branch from 6e10c40 to ec73698 Compare January 3, 2020 23:30

ariard force-pushed the 2019-11-fix-final-wallet branch 2 times, most recently from bbdc8b5 to fa5b990 Compare January 7, 2020 21:35

ryanofsky approved these changes Jan 8, 2020

View reviewed changes

ariard force-pushed the 2019-11-fix-final-wallet branch from fa5b990 to 9afb8dd Compare January 16, 2020 23:09

ariard commented Jan 16, 2020

View reviewed changes

ryanofsky approved these changes Jan 16, 2020

View reviewed changes

fjahr approved these changes Jan 18, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ariard mentioned this pull request Jan 21, 2020

wallet: Remove calls to Chain::Lock methods #17954

Merged

DrahtBot added the Needs rebase label Jan 30, 2020

ariard force-pushed the 2019-11-fix-final-wallet branch from 9afb8dd to ba18c2c Compare February 17, 2020 03:28

DrahtBot removed the Needs rebase label Feb 17, 2020

ryanofsky approved these changes May 13, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

jonatack reviewed May 15, 2020

View reviewed changes

maflcko mentioned this pull request May 15, 2020

doc: add note about considering making issues over TODO source code comments #18981

Closed

DrahtBot mentioned this pull request May 15, 2020

wallet: Minimal fix to restore conflicted transaction notifications #18982

Merged

ariard force-pushed the 2019-11-fix-final-wallet branch from b14e07b to 808dc17 Compare May 19, 2020 08:12

ryanofsky approved these changes May 19, 2020

View reviewed changes

ryanofsky reviewed May 19, 2020

View reviewed changes

DrahtBot mentioned this pull request May 27, 2020

refactor: replace pointers by references within tx_verify.{h,cpp} #19069

Merged

DrahtBot added the Needs rebase label Jun 2, 2020

Antoine Riard added 4 commits June 3, 2020 21:06

[consensus] Move IsFinalTx from tx_verify.h to tx_check.h

b0294d3

Context-independent IsFinalTx doesn't rely on chain or mempool state so moving it to tx_check let it being used by wallet code without linking server code in following commits.

[wallet] Add m_last_block_mediam_time_past in CWallet

1d2345b

Extend Notifications::BlockConnected/BlockDisconnected with median_time_past parameter. Used in next commit.

ariard force-pushed the 2019-11-fix-final-wallet branch from 808dc17 to d55ba31 Compare June 4, 2020 02:06

DrahtBot added Needs rebase and removed Needs rebase labels Jun 4, 2020

ryanofsky approved these changes Jun 12, 2020

View reviewed changes

ariard mentioned this pull request Aug 19, 2021

RBF move 2/3: extract RBF logic into policy/rbf #22675

Merged

maflcko closed this Mar 21, 2022

bitcoin locked and limited conversation to collaborators Apr 3, 2023

Drop checkFinalTx and use Median Time Past to check finality of wallet transactions #17443

Drop checkFinalTx and use Median Time Past to check finality of wallet transactions #17443

Uh oh!

Conversation

ariard commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Dec 18, 2019

Uh oh!

ariard commented Dec 18, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented May 19, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ariard commented Nov 11, 2019 •

edited

Loading

DrahtBot commented Nov 11, 2019 •

edited

Loading

ariard commented Jan 3, 2020 •

edited

Loading

ryanofsky Jan 16, 2020 •

edited

Loading