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

Remove GetDepthInMainChain dependency on locked chain interface #15931

Merged
merged 9 commits into from Nov 8, 2019

Conversation

@ariard
Copy link
Contributor

ariard commented May 1, 2019

Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.

I think it's ready for a first round of review before to get further.

  • BlockUntilSyncedToCurrent : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.

- AbandonTransaction : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed After #16624, we instead rely on Confirmation.

- AddToWalletIfInvolvingMe: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in mapWallet with a height set to zero so we remove check on block_hash.IsNull Already done in #16624

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented May 1, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17369 (Refactor: Move encryption code between KeyMan and Wallet by achow101)
  • #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
  • #17060 (Cache 26% more coins: Reduce CCoinsMap::value_type from 96 to 76 bytes by martinus)
  • #16944 (gui: create PSBT with watch-only wallet by Sjors)
  • #16910 (wallet: reduce loading time by using unordered maps by achow101)
  • #16895 (External signer multisig support by Sjors)
  • #16688 (log: Add validation interface logging by jkczyz)
  • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
  • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
  • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
  • #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch from 5653f46 to a0ef689 May 2, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 2, 2019
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch 3 times, most recently from adf914a to 773da90 May 6, 2019
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch from 773da90 to 8b66249 May 13, 2019
@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels May 13, 2019
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch from 8b66249 to e284e42 May 29, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 29, 2019
@fanquake fanquake added this to Chasing Concept ACK in High-priority for review Jun 19, 2019
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jun 19, 2019

Added to the "Chasing Concept ACK" board. @ryanofsky maybe you could give an initial Concept/Approach ACK/NACK ?

@ariard

This comment has been minimized.

Copy link
Contributor Author

ariard commented Jun 19, 2019

Yes would be awesome to get a Concept ACK on the approach followed but if it's not the best one, I'm eager to rework the PR in depth! Going to rebase/fix tests failure soon

Copy link
Contributor

ryanofsky left a comment

Concept ACK. I left a lot of suggestions, but overall this looks very good, and it's great to get rid of all these cs_main locks.

src/wallet/wallet.h Outdated Show resolved Hide resolved
@@ -351,6 +351,11 @@ class CMerkleTx
*/
int nIndex;

/* Refers to height of block against which tx is merkle branch linked to
* An block_height == 0 means that tx isn't yet linked to any block

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2019

Contributor

In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92)

Can we use -1 instead of 0 for this to be consistent with nIndex? Also would be good to note here that when a transaction is conflicted, hashBlock and block_height refer to the earliest block with a conflicting transaction instead of the block containing the transaction.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 23, 2019

Member

The comment says An block_height == 0 in commit Add m_block_height field in CMerkleTx and then changes to A block_height == -1 in Use CWallet::m_last_block_processed_height in GetDepthInMainChain.

It would be better to not change this in the course of the PR and just set it to A block_height == -1 in the first PR.

@@ -383,9 +389,10 @@ class CMerkleTx
READWRITE(hashBlock);
READWRITE(vMerkleBranch);
READWRITE(nIndex);
READWRITE(m_block_height);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2019

Contributor

In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92)

Isn't this going to break existing serialization of transactions in the wallet? I think it'd be best not to change the serialized representation and just make this a memory-only field that gets filled when the wallet is loaded.

This comment has been minimized.

Copy link
@ariard

ariard Jul 9, 2019

Author Contributor

Hmmm filled at startup using getBlockHeight, given we already have hashBlock ? Do you envision the wallet to always query chain state to succeed its startup ?

wtx.m_block_height = wtxIn.m_block_height;
fUpdated = true;
}
if (wtxIn.hashBlock.IsNull() && !wtx.hashBlock.IsNull()) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2019

Contributor

In commit "Add m_block_height field in CMerkleTx" (8a19755)

This logic is getting hard to follow with three repetitive if(hashBlock...) blocks. Now that this is unconditionally updating hashBlock, I think the new logic is equivalent to the following and can be simplified:

assert(!wtxIn.isAbandoned()); // Incoming transactions should never have special abandoned block hash
if (wtxIn.hashBlock != wtx.hashBlock) {
    wtx.hashBlock = wtxIn.hashBlock;
    wtx.m_block_height = wtxIn.m_block_height;
    fUpdated = true;
}

This comment has been minimized.

Copy link
@ariard

ariard Jul 9, 2019

Author Contributor

Updated AddToWallet, hope it's clearer

{
// Update the tx's hashBlock
hashBlock = block_hash;

// set the position of the transaction in the block
nIndex = posInBlock;

// set tx height
m_block_height = block_height;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2019

Contributor

In commit "Add m_block_height field in CMerkleTx" (8a19755)

It looks like a remaining place where m_block_height is still unset is in importprunedfunds. It would probably be good to change that code to call wtx.SetMerkleBranch instead of setting members directly.

This comment has been minimized.

Copy link
@ariard

ariard Jul 9, 2019

Author Contributor

Oooops, I forgot this one. I've added a call to SetMerkleBranch in importprunedfunds but had to change its argument to hask for transaction height too. Seems to me similar to the serialization issue, do we want wallet to ask chain the state of its stored transactions every time, even it should be able to infer it from the blocks already connected ?

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/interfaces/chain.h Show resolved Hide resolved
@@ -4448,10 +4448,18 @@ void CMerkleTx::SetMerkleBranch(const uint256& block_hash, int posInBlock, int b

int CMerkleTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
{
if (pwallet == nullptr)
return 0;

if (hashUnset())
return 0;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2019

Contributor

In commit "Use CWallet::m_last_block_processed_height in GetDepthInMainChain" (233ae92)

Can we assert(m_block_height > 0) after this point? I'm afraid of another bug like the importprunedfunds one mentioned earlier leaving m_block_height unset and this function returning bogus values.

This comment has been minimized.

Copy link
@ariard

ariard Jul 9, 2019

Author Contributor

Hmmm, I introduced assert(m_block_height >= -1) as we may GetDepthInMainChain on fresh transactions, their hashBlock being unset, their depth should appear as zero.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.h Outdated Show resolved Hide resolved
Copy link
Member

promag left a comment

Concept ACK.

@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch 2 times, most recently from cc9bd04 to 1458ded Jul 9, 2019
@ariard

This comment has been minimized.

Copy link
Contributor Author

ariard commented Jul 9, 2019

Answered back to @ryanofsky comments on 1458ded, I'm on cleaning last Travis failures

@jnewbery jnewbery mentioned this pull request Jul 9, 2019
@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Jul 9, 2019
@fanquake fanquake removed this from Chasing Concept ACK in High-priority for review Jul 11, 2019
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch 2 times, most recently from 277ebd4 to 04b4683 Jul 15, 2019
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch from f1d66ce to 8a11ab3 Nov 5, 2019
Copy link
Contributor

ryanofsky left a comment

Code review ACK 8a11ab3. No changes since previous review other than rebase

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 5, 2019

Sure Russell, I'll review.

Copy link
Member

jnewbery left a comment

utACK 8a11ab3

I have a couple of nits inline, which can be fixed up later in order to not invalidate review. There are a couple of changes that I would like fixed before merge, since they're to do with the git history, so can't be fixed after the fact (and they don't change the final code, so should be an easy re-ACK for other reviewers)

  1. Add the block_height member in the Add block_height field in struct Confirmation to not potentially break a git bisect and make the history correct.
  2. The wording for commit Restrain waitForNotificationsIfNewBlocksConnected check to strict tip is a little confusing. Can I suggest the following instead:
Only return early from BlockUntilSyncedToCurrentChain() if current tip is exact match

In the next commit, we start using BlockConnected/BlockDisconnected
callbacks to establish tx depth, rather than querying the chain
directly.

Currently, BlockUntilSyncedToCurrentChain() will return early if the
best block processed by the wallet is a descendant of the node's tip.
That means that in the case of a re-org, it won't wait for the
BlockDisconnected callbacks that have been enqueued during the re-org
but have not yet been triggered in the wallet.

Change BlockUntilSyncedToCurrentChain() to only return early if the
wallet's m_last_block_processed matches the tip exactly. This ensures
that there are no BlockDisconnected or BlockConnected callbacks
in-flight.

Great work with this PR. Thanks for persisting with it, and thanks to @ryanofsky for chasing reviewers!

src/wallet/wallet.h Show resolved Hide resolved
src/wallet/wallet.h Outdated Show resolved Hide resolved
src/wallet/wallet.h Outdated Show resolved Hide resolved
uint256 hashBlock;
int block_height;
int nIndex;
Confirmation(Status s = UNCONFIRMED, uint256 h = uint256(), int b = 0, int i = 0) : status(s), hashBlock(h), block_height(b), nIndex(i) {}

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 5, 2019

Member

I'm not a big fan of these default arguments, especially since the last two are both ints. It's too easy to accidentally call the constructor with the wrong number of arguments and set block_height when you meant to set nIndex (as this branch does in an intermediate commit)

This comment has been minimized.

Copy link
@ariard

ariard Nov 6, 2019

Author Contributor

Rearrange members initialization order and add comments, tell me if you have a better measure.

Copy link
Contributor

jkczyz left a comment

utACK 8a11ab3 modulo a few comments.

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
Status status;
uint256 hashBlock;
int block_height;
int nIndex;
Comment on lines 364 to 368

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 5, 2019

Contributor

Can these fields be const now?

This comment has been minimized.

Copy link
@ariard

ariard Nov 6, 2019

Author Contributor

I think we can't due to setting them with method like setAbandoned?

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 6, 2019

Contributor

Ah, you're right. Could be better to assign a new object within setAbandoned instead, but feel free to leave as it is. Looks like there are other places where these are directly assigned.

int nIndex = 0;
Status status;
uint256 hashBlock;
int block_height;

This comment has been minimized.

Copy link
@jkczyz

jkczyz Nov 5, 2019

Contributor

The documentation needs to be updated for this new field.

This comment has been minimized.

Copy link
@ariard

ariard Nov 6, 2019

Author Contributor

Shouldn't be an issue anymore as block_height has been switched to the right commit.

@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch 2 times, most recently from 520ab9e to 9810862 Nov 6, 2019
@ariard

This comment has been minimized.

Copy link
Contributor Author

ariard commented Nov 6, 2019

Updated at 36b68de :

  • move block_height to Add block_height field in struct Confirmation
  • modify Confirmation constructor and add comments to avoid ambiguity in place where it's called
  • rearrange Confirmation constructor comment
  • fix nit wording on Confirmation comment
  • add guard clause for importprunedfunds as suggested in its own commit
  • rewrite commit message for former Restrain waitForNotificationsIfNewBlocksConnected check to strict tip

Thanks @ryanofsky, @jnewbery, @jkczyz for reviews! Re-ACK should be minimal on diff.

EDIT: forgot to update test too, travis should pass now.

ariard added 6 commits Apr 20, 2019
At wallet loading, we rely on chain state querying to retrieve
height of txn, to do so we ensure that lock order is respected
between cs_main and cs_wallet.

If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as confirmation
height is not used by wallet-tool.

Reorder arguments and document Confirmation calls to avoid
ambiguity.

Fixes nits left from #16624
Avoid to lock chain to query state thanks to tracking last block
height in CWallet.
We don't remove yet Chain locks as we need to preserve lock
order with CWallet one until swapping at once to avoid
deadlock failures (spotted by --enable-debug)
Pass conflicting height in CWallet::MarkConflicted
is exact match

In the next commit, we start using BlockConnected/BlockDisconnected
callbacks to establish tx depth, rather than querying the chain
directly.

Currently, BlockUntilSyncedToCurrentChain will return early if
the best block processed by the wallet is a descendant of the node'tip.
That means that in the case of a re-org, it won't wait for the
BlockDisconnected callbacks that have been enqueued during the re-org
but have not yet been triggered in the wallet.

Change BlockUntilSyncedToCurrentChain to only return early if the
wallet's m_last_block_processed matches the tip exactly. This ensures
that there are no BlockDisconnected or BlockConnected callbacks
in-flight.
@ariard ariard force-pushed the ariard:2019-04-remove-external-locking branch from 9810862 to 36b68de Nov 6, 2019
Copy link
Contributor

ryanofsky left a comment

Code review ACK 36b68de. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment

Copy link
Member

jnewbery left a comment

utACK 36b68de

I've left one nit, which you definitely shouldn't fix unless you need to change something else.

int block_height;
uint256 hashBlock;
int nIndex;
Confirmation(Status s = UNCONFIRMED, int b = 0, uint256 h = uint256(), int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {}

This comment has been minimized.

Copy link
@jnewbery

jnewbery Nov 6, 2019

Member

very minor nit: using b for height and h for hash is a little confusing. Using a few extra characters and calling these status_in, height_in, hash_in and index_in would make this clearer.

@jkczyz

This comment has been minimized.

Copy link
Contributor

jkczyz commented Nov 6, 2019

utACK 769ff05

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 6, 2019

@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de).

@jkczyz

This comment has been minimized.

Copy link
Contributor

jkczyz commented Nov 7, 2019

@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de).

utACK 36b68de

Right! The PR interface was showing a different order from the branch.

Copy link
Member

promag left a comment

Code review ACK 36b68de.

Build some intermediate commits too. Some comments for your consideration.

@@ -1425,6 +1425,7 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
// to abandon a transaction and then have it inadvertently cleared by
// the notification that the conflicted transaction was evicted.

m_last_block_processed_height = height;

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

5aacc3e

Yes it has to be before

Could leave a comment explaining why?

Moved it near to m_last_block_processed_height.

Why? If there's no reason to change then I'd it keep where it was.

* without relying on Chain interface beyond asynchronous updates. For safety, we
* initialize it to -1.
*/
int m_last_block_processed_height GUARDED_BY(cs_wallet) = -1;

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

5aacc3e

nit, use default member initialize {-1} instead of = -1.

return m_last_block_processed_height;
};
/** Set last block processed height, currently only use in unit test */
void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

5aacc3e

Use const uint256& block_hash?

@@ -139,10 +139,12 @@ void TestGUI(interfaces::Node& node)
wallet->LoadWallet(firstRun);
{
auto spk_man = wallet->GetLegacyScriptPubKeyMan();
auto locked_chain = wallet->chain().lock();

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

5aacc3e

Why not just lock ::cs_main if you don't even use the interface?

This comment has been minimized.

Copy link
@ariard

ariard Nov 7, 2019

Author Contributor

Method chain here is the way to use the interfaces::chain, I think it doesn't matter that much here to use the Node or Chain one.

@@ -185,7 +185,7 @@ void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBloc
}
}

void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock)
void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

10b4729

nit, just pindex? Otherwise use snake case. Same in the header.

CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), txnIndex);
wtx.m_confirm = confirm;

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

9700fcb

You can also drop Confirmation constructor and do

wtx.m_confirm = {CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), int(txnIndex)};

and

    SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* hash = */ {}, /* index = */ 0);
if (isUnconfirmed() || isAbandoned()) return 0;

return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);
return (pwallet->GetLastBlockHeight() - m_confirm.block_height + 1) * (isConflicted() ? -1 : 1);

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

0ff0387

I don't think commit message is correct:

Avoid to lock chain to query state thanks to tracking last block
height in CWallet.

Should say something like?

Use wallet's last block height instead of the locked chain, which
will allow removing the chain lock.

This comment has been minimized.

Copy link
@ariard

ariard Nov 7, 2019

Author Contributor

That's a better wording, I will use it if I have to rebase.

@@ -986,7 +986,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
auto locked_chain = chain().lock();
LOCK(cs_wallet);

int conflictconfirms = -locked_chain->getBlockDepth(hashBlock);
int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1;

This comment has been minimized.

Copy link
@promag

promag Nov 7, 2019

Member

36b68de

What's the problem with -(m_last_block_processed_height - conflicting_height + 1)?

@ariard

This comment has been minimized.

Copy link
Contributor Author

ariard commented Nov 7, 2019

Can someome relaunch AppVeyor ? It got a linker error on bech32_tests.obj

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Nov 7, 2019

Can someome relaunch AppVeyor ? It got a linker error on bech32_tests.obj

Appveyor is an unrelated pending issue from #17357 affecting all other PRs that iiuc should be fixed by #17384.

@meshcollider

This comment has been minimized.

Copy link
Member

meshcollider commented Nov 8, 2019

Looks good, thanks for keeping this maintained ariard. I agree with most of promag's comments but they aren't worth holding up merge for.

utACK 36b68de

meshcollider added a commit that referenced this pull request Nov 8, 2019
…nterface

36b68de Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429 Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff0387 Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05 Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d38 Add block_height field in struct Confirmation (Antoine Riard)
9700fcb Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3e Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)

Pull request description:

  Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.

  I think it's ready for a first round of review before to get further.

  - `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.

  ~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.

  ~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624

ACKs for top commit:
  jnewbery:
    @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de).
  jkczyz:
    > @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](36b68de)).
  meshcollider:
    utACK 36b68de
  ryanofsky:
    Code review ACK 36b68de. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
  jnewbery:
    utACK 36b68de
  promag:
    Code review ACK 36b68de.

Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
@meshcollider meshcollider merged commit 36b68de into bitcoin:master Nov 8, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake removed this from Blockers in High-priority for review Nov 8, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2019

@shesek

This comment has been minimized.

Copy link

shesek commented Nov 19, 2019

It appears like @MarcoFalke's link always displays the last 10 days, which already don't include the performance gains (which happened on Nov 8). I couldn't find a way to link to a specific date range in codespeed, so for future reference for people bumping into this thread, here's a screenshot instead (of the last 50 days).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.