Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: master
from

wallet: Avoid use of Chain::Lock in listsinceblock

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
#14338 (comment)
  • Loading branch information
ryanofsky committed Jan 21, 2020
commit e276b6821430ec2c18aba55137daf98bae770054
@@ -259,6 +259,15 @@ class ChainImpl : public Chain
}
return true;
}
uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) override

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

nit: The other functions around here follow a different style, returning a bool. I would have preferred to keep this consistent.

{
LOCK(::cs_main);
if (const CBlockIndex* block = LookupBlockIndex(block_hash)) {
const CBlockIndex* ancestor = block->GetAncestor(ancestor_height);
if (ancestor) return ancestor->GetBlockHash();
}
return {};
}
bool findAncestorByHash(const uint256& block_hash, const uint256& ancestor_hash, int* height) override
{
LOCK(::cs_main);
@@ -268,6 +277,22 @@ class ChainImpl : public Chain
if (height) *height = ancestor->nHeight;
return true;
}
Optional<int> findCommonAncestor(const uint256& block_hash1,
const uint256& block_hash2,
uint256* ancestor_hash,
int* ancestor_height) override
{
LOCK(::cs_main);
const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
if (block1 && block2) {
const CBlockIndex* ancestor = LastCommonAncestor(block1, block2);
if (ancestor_hash) *ancestor_hash = ancestor->GetBlockHash();
if (ancestor_height) *ancestor_height = ancestor->nHeight;
return block1->nHeight;
}
return nullopt;
}
void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
double guessVerificationProgress(const uint256& block_hash) override
{
@@ -133,12 +133,23 @@ class Chain
int64_t* max_time = nullptr,
int64_t* mtp_time = nullptr) = 0;

//! Find ancestor of block at specified height and return its hash.
virtual uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) = 0;

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

e276b68

Why not modify getBlockHash a bit to do a LookupBlockIndex instead of querying ChainActive() ? Every block in ChainActive needs a BlockIndex so second should be a superset and it shouldn't be an issue.

If caller care about block being in the active chain, it should call findFork just after.

(Long-term, IMO wallet shouldn't have to deal with fork and just have a linear view of the chain, only when BlockDisconnected is called, state would be rewind. It's should be caller responsibility to enforce tips consistency between it's different components)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

e276b68

Why not modify getBlockHash a bit to do a LookupBlockIndex instead of querying ChainActive() ? Every block in ChainActive needs a BlockIndex so second should be a superset and it shouldn't be an issue.

If caller care about block being in the active chain, it should call findFork just after.

(Long-term, IMO wallet shouldn't have to deal with fork and just have a linear view of the chain, only when BlockDisconnected is called, state would be rewind. It's should be caller responsibility to enforce tips consistency between it's different components)

I'm removing getBlockHash in 3f1b867 from #15719.

I think of findAncestorByHeight as a more robust replacement for getBlockHash that returns the same thing reliably regardless of the chain tip. findAncestorByHeight is used in a few places. It's possible these could all go away in the future with your rescan branch, and by replacing listsinceblock and GetKeyBirthTimes code. The ugliest code is the rescan code. I'm not too worried about the other places, and I think none of the places involve wallet code that would be useful to run offline


//! Return whether block descends from a specified ancestor, and
//! optionally return height of the ancestor.
virtual bool findAncestorByHash(const uint256& block_hash,
const uint256& ancestor_hash,
int* height = 0) = 0;

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

nit: Maybe using std::numeric_limits<int>::max() would have been a tiny bit nicer because it would have allowed passing in a default initialized height.


//! Find most recent common ancestor between two blocks and optionally
//! return its hash and/or height. Also return height of first block. Return

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 19, 2020

Member

I think it's confusing to return something unrelated to the common ancestor here...

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 19, 2020

Author Contributor

re: #17954 (comment)

I think it's confusing to return something unrelated to the common ancestor here...

Agreed, will backport 25c1ae4 (branch) as soon as I get a chance

//! nullopt if either block is unknown or there is no common ancestor.
virtual Optional<int> findCommonAncestor(const uint256& block_hash1,

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

e276b68

"If both blocks are on the same chain ancestor_height is the height of the oldest between them. Also return height of first block which may be the same than ancestor height."

But honestly would prefer parameterize findFork instead of yet-another-single use method like passing wallet tip hash to findFork (and if null, then use default chain tip).

By the way, is findFork still used after this change ?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

e276b68

"If both blocks are on the same chain ancestor_height is the height of the oldest between them. Also return height of first block which may be the same than ancestor height."

But honestly would prefer parameterize findFork instead of yet-another-single use method like passing wallet tip hash to findFork (and if null, then use default chain tip).

By the way, is findFork still used after this change ?

findFork only used on startup after this change and is removed later in 3f1b867 from #15719.

I think findCommonAncestor is a more robust and more general version of findFork that works on any two blocks always returning the same value regardless of the current tip, avoiding race conditions that would otherwise happen when the tip is changing.

findCommonAncestor returns multiple values, so which of those values comes back in the return type, and which come back through output parameters is an aesthetic choice that isn't too important to me. Probably if we were using c++17 I would have this return a tuple. If you think it's bad to return block1 height, though, I could add a new int* block1_height output parameter, and change the return type from Optional<int> to bool.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

Here's a change that would make all the find block methods return block information same way 6f74c0a (branch), if it helps

EDIT: Newer version 25c1ae4

const uint256& block_hash2,
uint256* ancestor_hash,
int* ancestor_height) = 0;

//! Look up unspent output information. Returns coins in the mempool and in
//! the current chain UTXO set. Iterates through all the keys in the map and
//! populates the values.
@@ -2,7 +2,10 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <chainparams.h>
#include <consensus/validation.h>
#include <interfaces/chain.h>
#include <script/standard.h>
#include <test/util/setup_common.h>
#include <validation.h>

@@ -19,6 +22,14 @@ BOOST_AUTO_TEST_CASE(findBlock)
BOOST_CHECK_EQUAL(time_mtp, active[20]->GetMedianTimePast());
}

BOOST_AUTO_TEST_CASE(findAncestorByHeight)
{
auto chain = interfaces::MakeChain(m_node);
auto& active = ChainActive();
BOOST_CHECK_EQUAL(chain->findAncestorByHeight(active[20]->GetBlockHash(), 10), active[10]->GetBlockHash());
BOOST_CHECK_EQUAL(chain->findAncestorByHeight(active[10]->GetBlockHash(), 20), uint256());
}

BOOST_AUTO_TEST_CASE(findAncestorByHash)
{
auto chain = interfaces::MakeChain(m_node);
@@ -29,4 +40,26 @@ BOOST_AUTO_TEST_CASE(findAncestorByHash)
BOOST_CHECK(!chain->findAncestorByHash(active[10]->GetBlockHash(), active[20]->GetBlockHash()));
}

BOOST_AUTO_TEST_CASE(findCommonAncestor)
{
auto chain = interfaces::MakeChain(m_node);
auto& active = ChainActive();
auto* orig_tip = active.Tip();
for (int i = 0; i < 10; ++i) {
BlockValidationState state;
ChainstateActive().InvalidateBlock(state, Params(), active.Tip());
}
BOOST_CHECK_EQUAL(active.Height(), orig_tip->nHeight - 10);
coinbaseKey.MakeNewKey(true);
for (int i = 0; i < 20; ++i) {
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
}
BOOST_CHECK_EQUAL(active.Height(), orig_tip->nHeight + 10);
uint256 fork_hash;
int fork_height;
BOOST_CHECK_EQUAL(*chain->findCommonAncestor(orig_tip->GetBlockHash(), active.Tip()->GetBlockHash(), &fork_hash, &fork_height), orig_tip->nHeight);
BOOST_CHECK_EQUAL(fork_height, orig_tip->nHeight - 10);
BOOST_CHECK_EQUAL(fork_hash, active[fork_height]->GetBlockHash());
}

BOOST_AUTO_TEST_SUITE_END()
@@ -1585,8 +1585,9 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
uint256 blockId;
if (!request.params[0].isNull() && !request.params[0].get_str().empty()) {
blockId = ParseHashV(request.params[0], "blockhash");
height = locked_chain->findFork(blockId, &altheight);
if (!height) {
height.emplace();
altheight = pwallet->chain().findCommonAncestor(blockId, pwallet->GetLastBlockHash(), nullptr, height.get_ptr());
if (!altheight) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
}
}
@@ -1605,8 +1606,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)

bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());

const Optional<int> tip_height = locked_chain->getHeight();
int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
int depth = height ? pwallet->GetLastBlockHeight() + 1 - *height : -1;

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

e276b68

Height of genesis block is 0 ? If so depth is -1 which I think isn't the behavior expected (already there before ?)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

e276b68

Height of genesis block is 0 ? If so depth is -1 which I think isn't the behavior expected (already there before ?)

height is an Optional<int> so height ? is just checking if the optional value is set. If height is 0 the condition will evaluate to true and the correct depth should be set.

This comment has been minimized.

Copy link
@ariard

ariard Feb 14, 2020

Contributor

Oh right, it's an Optional, forget about it, forgive my C++ noobiness


UniValue transactions(UniValue::VARR);

@@ -1638,8 +1638,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
--*altheight;
}

int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
uint256 lastblock = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms);

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

e276b68

If I understand issue linked in the commit message, let's say you call listsinceblock(genesis_hash, 100) with current_tip == 1100 (shouldn't matter referring to chain_tip or wallet_tip).
Target_confirm = 1100 + 1 - 100 = 1001.
Lastblock = blockhash(1001)

Now while calling again listsinceblock(lastblock_hash, 100) with current_tip = 1100
depth = 1100 + 1 - 1001 = 100
So only transactions with depth < 100 are returned and not the ones with 100-conf as expected by target_confirmations (i.e transactions for block 1101, the "100th" from the main chain).

Is this the behavior you're fixing by returning now the ancestor hash? Seems to me documentation is already marching code "So you would generally use a target_confirmations of say 6, you will be continually re-notified of transactions until they've reached 6 confirmations plus any new ones" but not sure if this what we really want..

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

e276b68

If I understand issue linked in the commit message, let's say you call listsinceblock(genesis_hash, 100) with current_tip == 1100 (shouldn't matter referring to chain_tip or wallet_tip).
Target_confirm = 1100 + 1 - 100 = 1001.
Lastblock = blockhash(1001)

Now while calling again listsinceblock(lastblock_hash, 100) with current_tip = 1100
depth = 1100 + 1 - 1001 = 100
So only transactions with depth < 100 are returned and not the ones with 100-conf as expected by target_confirmations (i.e transactions for block 1101, the "100th" from the main chain).

Is this the behavior you're fixing by returning now the ancestor hash? Seems to me documentation is already marching code "So you would generally use a target_confirmations of say 6, you will be continually re-notified of transactions until they've reached 6 confirmations plus any new ones" but not sure if this what we really want..

I think I need to reread your example more closely to give a better response, but the case which this commit should fix is specifically the case where wallet_tip != chain_tip. So if the wallet is behind and wallet_tip=1100 while chain_tip=1150, I want the first listsinceblock call to return lastblock=blockhash(1001) instead of blockhash(1051) so transactions aren't missed in the second call


UniValue ret(UniValue::VOBJ);
ret.pushKV("transactions", transactions);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.