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: Add BlockUntilSyncedToCurrentChain to dumpwallet #18671

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 16, 2020

dumpwallet includes the block hash in the output, so this method depends on the chainstate. According to the developer notes https://github.com/bitcoin/bitcoin/blame/e84a5f000493fe39adb2a5f22b43c3848dcd0a4f/doc/developer-notes.md#L1095 it must include a BlockUntilSyncedToCurrentChain.

It fixes test failures such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/675487097#L2657 , which can only happen in master because #17954 is only in master.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 16, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. Need to fix test since lock is held before:

::dumpwallet(request);

@promag
Copy link
Contributor

promag commented Apr 17, 2020

Code review ACK fa60afc.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa60afc

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa60afc

@meshcollider meshcollider merged commit e890c15 into bitcoin:master Apr 23, 2020
@maflcko maflcko deleted the 2004-walletDumpChain branch April 23, 2020 10:46
@luke-jr
Copy link
Member

luke-jr commented Apr 24, 2020

I don't see why we should ever hold a fix back from backporting... especially a simple one.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 24, 2020
…mpwallet

fa60afc wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet (MarcoFalke)

Pull request description:

  dumpwallet includes the block hash in the output, so this method depends on the chainstate. According to the developer notes https://github.com/bitcoin/bitcoin/blame/e84a5f000493fe39adb2a5f22b43c3848dcd0a4f/doc/developer-notes.md#L1095 it must include a `BlockUntilSyncedToCurrentChain`.

  This is a minor fix and does not need backport, I think.

  It fixes test failures such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/675487097#L2657 , which can only happen in master because the test was not backported.

ACKs for top commit:
  promag:
    Code review ACK fa60afc.
  ryanofsky:
    Code review ACK fa60afc
  meshcollider:
    utACK fa60afc

Tree-SHA512: 8df70b06b226b2cdf880dec9264adb72d66fd81b09b404fd1665a79e5f5236d26122eebf15df00fe71ee292b5c91b2dc23a0a42b2aa50a8d690604b23832723f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 5, 2020
Just the actual fix

Github-Pull: bitcoin#18671
Rebased-From: fa60afc (partial)
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 11, 2020
Summary:
This is a backport of Core [[bitcoin/bitcoin#18671 | PR18671]]

Depends on D7868

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7876
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants