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

Here it seems should be either "Setting-appropriate flags select" or "Setting appropriate flags selects".

I think it's the latter but it's not clear from the sentence.

Suggest: s/would/will/

s/i.e/i.e./

It looks like "softfork" should be plural.

Maybe drop this sentence about CSV/LOCKTIME_MEDIAN_TIME_PAST. It seems only slightly relevant, and already pretty clear by looking at the code below. This sentence could also become out of date if the code below changes.

I understand correcting "negative value for flags indicates that the current network-enforced consensus rules should be used" to "negative value for flags indicates that no soft-forked consensus rules should be used".

But I think the other changes to this comment make it less clear. I'm wondering if something about the following removed section is inaccurate or if it was shortened and rewritten for a different reason:

In a future soft-fork scenario that would mean checking which rules would be enforced for the next block and setting the appropriate flags. At the present time no soft-forks are scheduled, so no flags are set.

I think the comment was written when BIP113 code was merged but no yet activated and scheduled for activation. It's right we don't have soft-forks scheduled now, but it's false there is no flag to set (LOCKTIME_MEDIAN_TIME_PAST)

re: #17443 (review)

I don't understand "it's false there is no flag to set (LOCKTIME_MEDIAN_TIME_PAST)." since this code isn't setting LOCKTIME_MEDIAN_TIME_PAST, it's setting 0...

But more broadly, I don't understand why there would be a reason to keep this code if it isn't going to serve a function in a future soft fork. Why not simplify by making the CheckFinalTx flags argument required instead of optional, and dropping std::max. and dropping this comment?

CheckFinalTx is called with STANDARD_LOCKTIME_VERIFY_FLAGS in PreChecks and removeForReorg. Given it's only mempool I think you'e right, you can hardcode that you always want to use the most recent consensus rules but I would prefer to do that as a follow-up to avoid missing something.

In commit "Replace Chain::checkFinal by CWallet::CheckFinalWalletTx" (0ef43cef24b837367a90c97a31470e0f19f18552)

I see this comment is copied from existing code, but the part about this function being called from CBlock::AcceptBlock doesn't apply here, right? Would it be ok to drop this long comment and simply write that we are passing m_last_block_processed_height + 1 because we are interested to know if the transaction could be included in the next block, not the current tip?

The comment is about that we want to call IsFinalTx the same way it's called in AcceptBlock, i.e with nBlockHeight set to tip+1 to evaluate the consensus rule identically in both wallet and node. You don't want discrepancies between them. I think the last sentence is expressing that "if a transaction can be part of the next block", and speaking of m_last_block_processed_height is verbose but at least you avoid confusion between wallet tip and node tip.

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;
 };

nit: some grammar improvement on this paragraph

    // The wallet is not doing historical validation work, it's only
    // interested in the nLocktime evaluation as enforced on
    // the current chain tip (BIP 113). If there are forks changing
    // this in the future, the wallet should be updated.

Thanks for review, will take them if I have to update PR!

It would be good to fix this. I started rewriting it as well before seeing @fjahr's suggestion. It's costing multiple reviewers time already (and future readers trying to parse it) when it could be fixed easily.

Thanks, took comment.

In commit "Add m_last_block_mediam_time_past in CWallet" (46744a2998ee196bd366b49fa14036ab7acec30b)

Would seem better to handle failure, drop temporary variable, and use m_last_block_processed consistently here:

CHECK_NONFATAL(chain.findBlock(walletInstance->m_last_block_processed, FoundBlock().mtpTime(walletInstance->m_last_block_median_time_past)));

46744a2 I agree with @ryanofsky's suggestion above.

If you do keep the tempvar, please move it to the local scope where it is used:

-    int64_t mtp_time;
     if (tip_height) {
         walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
         walletInstance->m_last_block_processed_height = *tip_height;
+        int64_t mtp_time;

Solved, note I don't think using m_last_block_processed or GetBlockHash changes something on the consistency level but did so for code clarity

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)

36ab1db7 Why not resolve the issue and get rid of this (and the TODO), rather than taking on technical debt?

Yes thanks for the call, in fact issue was solved since. Removed.

Nice improvement here!

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

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

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