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

ariard commented at 6:37 pm on November 11, 2019: member

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.

in src/validation.cpp:197 in c960973720 outdated

198-    // rules would be enforced for the next block and setting the
199-    // appropriate flags. At the present time no soft-forks are
200-    // scheduled, so no flags are set.
201+    // By convention a negative value for flags indicates that no
202+    // soft-forked consensus rules should be used. Setting appropriate
203+    // flags select which rules would be enforced for the next block.

jonatack commented at 6:47 pm on November 11, 2019:

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/

in src/validation.cpp:198 in c960973720 outdated

199-    // appropriate flags. At the present time no soft-forks are
200-    // scheduled, so no flags are set.
201+    // By convention a negative value for flags indicates that no
202+    // soft-forked consensus rules should be used. Setting appropriate
203+    // flags select which rules would be enforced for the next block.
204+    // At the present time, only CSV softfork (i.e BIP 113) alter

jonatack commented at 6:49 pm on November 11, 2019:

s/i.e/i.e./

It looks like “softfork” should be plural.

jonatack commented at 6:55 pm on November 11, 2019: member

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.

DrahtBot added the label Validation on Nov 11, 2019

DrahtBot commented at 7:38 pm on November 11, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19069 (refactor: replace pointers by references within tx_verify.{h,cpp} by theStack)
#18096 (doc: IsFinalTx comment about nSequence & OP_CLTV by nothingmuch)
#17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)
#15719 (Wallet passive startup by ryanofsky)
#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.

jnewbery removed the label Validation on Nov 17, 2019

jnewbery added the label Wallet on Nov 17, 2019

in src/validation.cpp:199 in c960973720 outdated

200-    // scheduled, so no flags are set.
201+    // By convention a negative value for flags indicates that no
202+    // soft-forked consensus rules should be used. Setting appropriate
203+    // flags select which rules would be enforced for the next block.
204+    // At the present time, only CSV softfork (i.e BIP 113) alter
205+    // nBlockTime meaning through LOCKTIME_MEDIAN_TIME_PAST flag.

ryanofsky commented at 4:22 pm on December 16, 2019:

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.

in src/validation.cpp:211 in c960973720 outdated

197-    // a future soft-fork scenario that would mean checking which
198-    // rules would be enforced for the next block and setting the
199-    // appropriate flags. At the present time no soft-forks are
200-    // scheduled, so no flags are set.
201+    // By convention a negative value for flags indicates that no
202+    // soft-forked consensus rules should be used. Setting appropriate

ryanofsky commented at 4:32 pm on December 16, 2019:

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.

ariard commented at 0:55 am on December 18, 2019:

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)

ryanofsky commented at 6:32 pm on December 18, 2019:

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?

ariard commented at 0:25 am on December 19, 2019:

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.

ryanofsky approved

ryanofsky commented at 5:09 pm on December 16, 2019: member

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 (review)). This could also be added as another Chain interface TODO item if it is not worth the effort to implement now.

ariard force-pushed on Dec 18, 2019

ariard renamed this:
~~Use Median Time Past to check finality of wallet transactions~~
Drop checkFinalTx and use Median Time Past to check finality of wallet transactions
on Dec 18, 2019

ariard commented at 0:49 am on December 18, 2019: member

@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 force-pushed on Dec 18, 2019

ariard commented at 6:28 pm on December 18, 2019: member

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.

in src/wallet/wallet.cpp:4162 in 0ef43cef24 outdated

3981+    // to evaluate nLockTime because when IsFinalTx() is called within
3982+    // CBlock::AcceptBlock(), the height of the block *being*
3983+    // evaluated is what is used. Thus if we want to know if a
3984+    // transaction can be part of the *next* block, we need to call
3985+    // IsFinalTx() with one more than m_last_block_processed_height.
3986+    return IsFinalTx(tx, m_last_block_processed_height + 1, m_last_block_median_time_past);

ryanofsky commented at 6:44 pm on December 18, 2019:

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?

ariard commented at 0:29 am on December 19, 2019:

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.

ryanofsky approved

ryanofsky commented at 6:51 pm on December 18, 2019: member

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

ariard force-pushed on Dec 19, 2019

ariard commented at 1:09 am on December 19, 2019: member

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

ryanofsky approved

ryanofsky commented at 5:08 pm on December 19, 2019: member

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

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

ariard force-pushed on Dec 19, 2019

ariard force-pushed on Jan 3, 2020

ariard commented at 11:32 pm on January 3, 2020: member

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

ariard force-pushed on Jan 4, 2020

ariard force-pushed on Jan 7, 2020

in src/wallet/wallet.h:497 in 1f479751b2 outdated

476@@ -477,8 +477,8 @@ class CWalletTx
477     bool IsEquivalentTo(const CWalletTx& tx) const;
478 
479     bool InMempool() const;
480-    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const;
481-    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const;
482+    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS;
483+    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const NO_THREAD_SAFETY_ANALYSIS;

ryanofsky commented at 10:28 pm on January 8, 2020:

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:

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 9d291f0b585..c702f90681d 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -1882,6 +1882,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const
 5 bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const
 6 {
 7     // Quick answer in most cases
 8+    AssertLockHeld(pwallet->cs_wallet);
 9     if (!pwallet->CheckFinalWalletTx(*tx)) return false;
10     int nDepth = GetDepthInMainChain();
11     if (nDepth >= 1) return true;
12@@ -3967,6 +3968,7 @@ bool CWalletTx::IsImmatureCoinBase() const
13 
14 bool CWalletTx::isFinal() const
15 {
16+    AssertLockHeld(pwallet->cs_wallet);
17     return pwallet->CheckFinalWalletTx(*tx);
18 }
19 
20diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
21index acf0de45f1c..4ae39ba328a 100644
22--- a/src/wallet/wallet.h
23+++ b/src/wallet/wallet.h
24@@ -477,8 +477,8 @@ public:
25     bool IsEquivalentTo(const CWalletTx& tx) const;
26 
27     bool InMempool() const;
28-    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS;
29-    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const NO_THREAD_SAFETY_ANALYSIS;
30+    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const;
31+    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const;
32 
33     int64_t GetTxTime() const;
34 
35@@ -531,7 +531,7 @@ public:
36     const uint256& GetHash() const { return tx->GetHash(); }
37     bool IsCoinBase() const { return tx->IsCoinBase(); }
38     bool IsImmatureCoinBase() const;
39-    bool isFinal() const NO_THREAD_SAFETY_ANALYSIS;
40+    bool isFinal() const;
41 };
42 
43 class COutput

6e10c40e9507c2ff45d47ca863778fff85584b36

ariard commented at 10:59 pm on January 16, 2020:

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

ryanofsky commented at 11:21 pm on January 16, 2020:

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.

 0--- a/src/wallet/wallet.h
 1+++ b/src/wallet/wallet.h
 2@@ -481,12 +481,12 @@ public:
 3     // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The
 4     // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid
 5     // having to resolve the issue of member access into incomplete type CWallet.
 6-    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS;
 7+    bool IsTrusted(interfaces::Chain::Lock& locked_chain) const;
 8     // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
 9     // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The
10     // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid
11     // having to resolve the issue of member access into incomplete type CWallet.
12-    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const NO_THREAD_SAFETY_ANALYSIS;
13+    bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const;
14 
15     int64_t GetTxTime() const;
16 
17@@ -543,7 +543,7 @@ public:
18     // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The
19     // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid
20     // having to resolve the issue of member access into incomplete type CWallet.
21-    bool isFinal() const NO_THREAD_SAFETY_ANALYSIS;
22+    bool isFinal() const;
23 };

ryanofsky approved

ryanofsky commented at 10:30 pm on January 8, 2020: member

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.

ariard force-pushed on Jan 16, 2020

ariard commented at 11:09 pm on January 16, 2020: member

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 approved

ryanofsky commented at 11:23 pm on January 16, 2020: member

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

in src/wallet/wallet.cpp:4145 in 9afb8ddd1d outdated

3974+}
3975+
3976+bool CWallet::CheckFinalWalletTx(const CTransaction& tx) const
3977+{
3978+    AssertLockHeld(cs_wallet);
3979+    // Wallet not doing historical validation work, it's only

fjahr commented at 9:39 pm on January 18, 2020:

nit: some grammar improvement on this paragraph

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

ariard commented at 9:30 pm on January 29, 2020:

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

jonatack commented at 8:58 am on May 15, 2020:

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.

ariard commented at 8:15 am on May 19, 2020:

Thanks, took comment.

fjahr approved

fjahr commented at 9:46 pm on January 18, 2020: member

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

DrahtBot added the label Needs rebase on Jan 30, 2020

ariard force-pushed on Feb 17, 2020

DrahtBot removed the label Needs rebase on Feb 17, 2020

ryanofsky approved

ryanofsky commented at 5:49 pm on February 24, 2020: member

Code review ACK ba18c2cb5c23c150e5a72c3594342bdd955ce3a7. No changes other than rebase due to conflict with #17261 (trivial, just adjacent lines)

DrahtBot added the label Needs rebase on Mar 19, 2020

ariard force-pushed on May 11, 2020

DrahtBot removed the label Needs rebase on May 11, 2020

ariard commented at 6:03 am on May 13, 2020: member

@fjahr @ryanofsky it has been rebased after #16426 merged, they shouldn’t have any substantial changes since your last ACK but let me know if you find any issue.

in src/wallet/wallet.cpp:3933 in 46744a2998 outdated

3925@@ -3924,12 +3926,16 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3926     }
3927 
3928     const Optional<int> tip_height = chain.getHeight();
3929+    int64_t mtp_time;
3930     if (tip_height) {
3931         walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
3932         walletInstance->m_last_block_processed_height = *tip_height;
3933+        chain.findBlock(walletInstance->GetLastBlockHash(), FoundBlock().mtpTime(mtp_time));

ryanofsky commented at 7:58 pm on May 13, 2020:

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:

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

jonatack commented at 8:16 am on May 15, 2020:

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:

0-    int64_t mtp_time;
1     if (tip_height) {
2         walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
3         walletInstance->m_last_block_processed_height = *tip_height;
4+        int64_t mtp_time;

ariard commented at 8:14 am on May 19, 2020:

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

ryanofsky approved

ryanofsky commented at 8:01 pm on May 13, 2020: member

Code review ACK b14e07b227f3830120e97f44b3d6ece55bf700c6. Just rebased since previous review

in src/consensus/tx_check.cpp:60 in b14e07b227 outdated

55@@ -56,3 +56,16 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
56 
57     return true;
58 }
59+
60+bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)

jonatack commented at 8:04 am on May 15, 2020:

a20187d suggestion while moving this code

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

in src/wallet/wallet.h:561 in b14e07b227 outdated

554@@ -555,6 +555,11 @@ class CWalletTx
555     // wrong copy.
556     CWalletTx(CWalletTx const &) = delete;
557     void operator=(CWalletTx const &x) = delete;
558+    // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
559+    // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The
560+    // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid
561+    // having to resolve the issue of member access into incomplete type CWallet.

jonatack commented at 8:47 am on May 15, 2020:

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

ariard commented at 8:14 am on May 19, 2020:

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

in src/wallet/rpcwallet.cpp:1044 in b14e07b227 outdated

1040@@ -1041,7 +1041,7 @@ static UniValue ListReceived(const CWallet* const pwallet, const UniValue& param
1041     for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
1042         const CWalletTx& wtx = pairWtx.second;
1043 
1044-        if (wtx.IsCoinBase() || !pwallet->chain().checkFinalTx(*wtx.tx)) {
1045+        if (wtx.IsCoinBase() || !wtx.isFinal()) {

jonatack commented at 9:13 am on May 15, 2020:

Nice improvement here!

jonatack commented at 9:29 am on May 15, 2020: member

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 (review), 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.

ariard force-pushed on May 19, 2020

ariard commented at 8:16 am on May 19, 2020: member

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

ryanofsky approved

ryanofsky commented at 11:58 am on May 19, 2020: member

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

in src/wallet/wallet.h:558 in 0387088196 outdated

554@@ -555,6 +555,7 @@ class CWalletTx
555     // wrong copy.
556     CWalletTx(CWalletTx const &) = delete;
557     void operator=(CWalletTx const &x) = delete;
558+    bool isFinal() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

ryanofsky commented at 12:05 pm on May 19, 2020:

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

Travis error here:

0In file included from interfaces/wallet.cpp:24:
1./wallet/wallet.h:558:51: error: use of undeclared identifier 'cs_wallet'; did you mean 'pwallet'?
2    bool isFinal() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
3                                                  ^~~~~~~~~
4                                                  pwallet
5./threadsafety.h:31:79: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
6#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
7                                                                              ^
8./wallet/wallet.h:272:20: note: 'pwallet' declared here
9    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

jonatack commented at 12:34 pm on May 19, 2020:

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

jonatack commented at 12:47 pm on May 19, 2020:

Building with pwallet->cs_wallet halts with

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

ariard commented at 10:39 pm on May 19, 2020:

@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..

ariard commented at 2:06 am on June 4, 2020:

Updated dropping isFinal accessor on the advice of @ryanofsky

jonatack commented at 12:51 pm on May 19, 2020: member

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

fjahr commented at 12:12 pm on May 23, 2020: member

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.

DrahtBot added the label Needs rebase on Jun 2, 2020

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

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.

b0294d38f5

[wallet] Add m_last_block_mediam_time_past in CWallet

Extend Notifications::BlockConnected/BlockDisconnected with
median_time_past parameter.

Used in next commit.

1d2345b31f

[wallet] Replace Chain::checkFinal by CWallet::CheckFinalWalletTx

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.

06a703a793

[validation] Fix CheckFinalTx comment ambiguity around softforks.

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.

d55ba31ddd

ariard force-pushed on Jun 4, 2020

ariard commented at 2:07 am on June 4, 2020: member

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

DrahtBot removed the label Needs rebase on Jun 4, 2020

DrahtBot added the label Needs rebase on Jun 8, 2020

DrahtBot commented at 4:34 pm on June 8, 2020: member

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

in src/wallet/wallet.cpp:3946 in 1d2345b31f outdated

3942@@ -3941,9 +3943,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3943     if (tip_height) {
3944         walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
3945         walletInstance->m_last_block_processed_height = *tip_height;
3946+        chain.findBlock(walletInstance->m_last_block_processed, FoundBlock().mtpTime(walletInstance->m_last_block_median_time_past));

ryanofsky commented at 2:25 am on June 12, 2020:

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

ryanofsky approved

ryanofsky commented at 2:52 pm on June 12, 2020: member

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

DrahtBot commented at 11:22 am on December 15, 2021: member

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 commented at 1:07 pm on March 21, 2022: member

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.

MarcoFalke commented at 1:22 pm on March 21, 2022: member

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

MarcoFalke closed this on Mar 21, 2022

MarcoFalke commented at 1:24 pm on March 21, 2022: member

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

ariard commented at 7:07 pm on April 3, 2022: member

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.

DrahtBot locked this on Apr 3, 2023

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

Conflicts