Skip to content

Conversation

@jamesob
Copy link
Contributor

@jamesob jamesob commented Jul 8, 2019

This is part of the assumeutxo project:

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal


This change moves CCoinsViewErrorCatcher out of init and into coins so that it can later be included in a CoinsView instance under CChainState.

Instead of hardcoding read failure behavior that has knowledge of qt, it accepts error callbacks via AddReadErrCallback().

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 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:

  • #16362 (gui: Bilingual translation by hebasto)
  • #15606 ([experimental] UTXO snapshots by jamesob)
  • #9384 (CCoinsViewCache code cleanup & deduplication 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.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK, just some dumb style nits and an erroneous include, I think. Feel free to ignore the nits.

@jamesob jamesob force-pushed the 2019-07-au-coinscatcher branch from afe7aff to 99ccdef Compare July 8, 2019 19:26
@jamesob
Copy link
Contributor Author

jamesob commented Jul 8, 2019

Took @MarcoFalke's patch and rebased. Thanks, Marco.

@jamesob jamesob force-pushed the 2019-07-au-coinscatcher branch from 99ccdef to 30de23d Compare July 8, 2019 19:36
@jamesob
Copy link
Contributor Author

jamesob commented Jul 8, 2019

Had to revert part of Marco's patch (see #16355 (comment)).

@maflcko
Copy link
Member

maflcko commented Jul 8, 2019

ACK 30de23d, mostly move-only

moving CCoinsViewErrorCatcher out of init makes sense, so that e.g. a chainstate
constructor can be written and called by unit tests.

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 30de23d743ca3d9b3335061bccf6cf8ab30a8b79, mostly move-only

moving CCoinsViewErrorCatcher out of init makes sense, so that e.g. a chainstate
constructor can be written and called by unit tests.

-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhU3QwAzwg4UXI93uSKkvTlg+mgt5Rf5Tmg1hSj1kvY7PoMH3JniaUiJLnHuAKn
a9Uoa1vzeu1WcBfcSxSn3RydEPDMT2TO7eRmzSWSfrBelie35uEWEaXOo7/hWdDK
UPm0CMb4jryEUyO/jD2jyP1qfEZnEuJfn1MAE9O0npTJbowtwlBS3s+Axhbl/bRg
Pao4A/IJJabHMnVJi8qTBXLnQhFT3VNylA+8gidVY9nuEi1+2VfFTzUTubEwaLBC
X3oTajj4IpTNhJjQV+vrFL8e9K0TOaS7e3PvY/6C1Jrme7oZ6lTvSTEf6pdYLj0U
86fyF7qKGJpSrbZ6c1tMz8uE9gNK175uuLh0QvMVORjNqfH3vRxfLyVq8Pbf+ZGj
KK69B5tqAZjtD3yDNVdT43HuT7L6Tl9pCt4O8I59vPO+mMMtkXP/mzdvOdCs5D5+
au//19ip8vu2nmCKqjjvywaC6GvDrGuLQffOdNoAmadYvsCyxjE1w4IgzC42Xz8Z
ivx+v3++
=Exqc
-----END PGP SIGNATURE-----

Timestamp of file with hash 8aaa47a81852504f0e86b718713f70725fbd3105bffb9d111c31263336e34a6d -

@dongcarl
Copy link
Contributor

ACK 30de23d

Code changes make sense and I tested locally.

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.

utACK 30de23d. Simple change. I verified the appropriate parts are move-only with -w --color-moved=dimmed_zebra.

src/coins.h Outdated
public:
explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}

void AddReadErrCallback(const std::function<void()> f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should drop const, otherwise std::move below can't really have any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@fanquake
Copy link
Member

utACK. Could you fix up Rus's nit and this should be mergeable.

and into coins.cpp. This move is necessary so that we can later include a
CCoinsViewErrorCatcher instance under CChainState.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
@jamesob jamesob force-pushed the 2019-07-au-coinscatcher branch from 30de23d to 4f050b9 Compare July 22, 2019 01:01
@jamesob
Copy link
Contributor Author

jamesob commented Jul 22, 2019

Thanks for the reviews and nudge. Pushed.

@dongcarl
Copy link
Contributor

re-ACK 4f050b9

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.

utACK 4f050b9. Only change since last review is fixing const.

@fanquake fanquake merged commit 4f050b9 into bitcoin:master Jul 23, 2019
fanquake added a commit that referenced this pull request Jul 23, 2019
4f050b9 move-onlyish: move CCoinsViewErrorCatcher out of init.cpp (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):

  Parent PR: #15606
  Issue: #15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal

  ---

  This change moves `CCoinsViewErrorCatcher` out of `init` and into `coins` so that it can later be included in [a `CoinsView` instance](9128496#diff-349fbb003d5ae550a2e8fa658e475880R504) under `CChainState`.

  Instead of hardcoding read failure behavior that has knowledge of qt, it accepts error callbacks via `AddReadErrCallback()`.

ACKs for top commit:
  dongcarl:
    re-ACK 4f050b9
  ryanofsky:
    utACK 4f050b9. Only change since last review is fixing const.

Tree-SHA512: eaba21606d15d2b8d0e3db7cec57779ce181af953db1ef4af80a0bc1dfb57923d0befde9d61b7be55c32224744f7fb6bd47d4e4c72f3ccfe6eaf0f4ae3765c17
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 18, 2020
Summary:
4f050b91c706181084b9288b8a87b7b637e4e4f7 move-onlyish: move CCoinsViewErrorCatcher out of init.cpp (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):

  Parent PR: #15606
  Issue: #15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal

  ---

  This change moves `CCoinsViewErrorCatcher` out of `init` and into `coins` so that it can later be included in [a `CoinsView` instance](bitcoin/bitcoin@9128496#diff-349fbb003d5ae550a2e8fa658e475880R504) under `CChainState`.

  Instead of hardcoding read failure behavior that has knowledge of qt, it accepts error callbacks via `AddReadErrCallback()`.

ACKs for top commit:
  dongcarl:
    re-ACK 4f050b91c706181084b9288b8a87b7b637e4e4f7
  ryanofsky:
    utACK 4f050b91c706181084b9288b8a87b7b637e4e4f7. Only change since last review is fixing const.

Tree-SHA512: eaba21606d15d2b8d0e3db7cec57779ce181af953db1ef4af80a0bc1dfb57923d0befde9d61b7be55c32224744f7fb6bd47d4e4c72f3ccfe6eaf0f4ae3765c17

Backport of Core [[bitcoin/bitcoin#16355 | PR16355]]

Test Plan:
  ninja
  ninja check-all
  src/bitcoind
Verify normal start-up and operation.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6634
kwvg added a commit to kwvg/dash that referenced this pull request Oct 22, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants