Don't return stale data from CCoinsViewCache::Cursor() by ryanofsky · Pull Request #10550 · bitcoin/bitcoin

ryanofsky · 2017-06-07T17:23:38Z

CCoinsViewCache doesn't actually support cursor iteration returning the
current contents of the cache, so raise an error when the cursor method is
called instead of returning a cursor that iterates over stale data.

Also update the gettxoutsetinfo RPC which was relying on the old behavior to be
explicit about which view it is returning data about.

CCoinsViewCache doesn't actually support cursor iteration returning the current contents of the cache, so raise an error when the cursor method is called instead of returning a cursor that iterates over stale data. Also update the gettxoutsetinfo RPC which was relying on the old behavior to be explicit about which view it is returning data about.

sipa · 2017-06-07T17:40:40Z

src/coins.h

    void SetBestBlock(const uint256 &hashBlock);
    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {
+        throw std::logic_error("CCCoinsViewCache cursor iteration not supported.");


Fixed in fe76b30

ryanofsky

Squashed fe76b30 -> c9d5b10 (pr/coins-stale.2 -> pr/coins-stale.3)

ryanofsky · 2017-06-07T20:22:25Z

src/coins.h

    void SetBestBlock(const uint256 &hashBlock);
    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {
+        throw std::logic_error("CCCoinsViewCache cursor iteration not supported.");


Fixed in fe76b30

jonasschnelli · 2017-06-08T06:23:41Z

utACK c9d5b10

laanwj · 2017-06-08T07:20:34Z

src/coins.h

    uint256 GetBestBlock() const;
    void SetBestBlock(const uint256 &hashBlock);
    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {


This 'override' is causing tons of warnings here (for every file that includes coins.h).

/home/orion/projects/bitcoin/bitcoin/src/coins.h:210:10: warning: 'GetCoin' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] bool GetCoin(const COutPoint &outpoint, Coin &coin) const;

Some compilers (clang5, in this case) demand that a whole class is annotated with it if it is used anywhere. So I'd argue against adding it here, or in a separate commit add it to all appropriate CCoinsViewCache methods.

Removed in c2a7640.

Added back overrides in a51ff25

laanwj · 2017-06-08T07:20:46Z

This makes a lot of sense, the use of API here has always bothered me but I wasn't sure how to fix it. Seems a good stopgap until we need #9306.
ACK (+ verified that gettxoutsetinfo fields that depend on this iteration display the same with and without this patch) apart from nit.

paveljanik · 2017-06-08T10:06:04Z

src/validation.h

 extern CChain chainActive;

+/** Global variable that points to the coins database (protected by cs_main) */
+extern CCoinsViewDB *pcoinsdbview;


Coins View DB vs coins db view?

Coins View DB vs coins db view?

Are you suggesting to rename the variable? If so I would slightly prefer not to, because this is a preexisting variable which this PR is just making public.

Agreed. Thanks for clarification.

ryanofsky

Squashed c2a7640 -> 24e44c3 (pr/coins-stale.4 -> pr/coins-stale.6)

ryanofsky · 2017-06-08T11:47:09Z

src/coins.h

    uint256 GetBestBlock() const;
    void SetBestBlock(const uint256 &hashBlock);
    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {


Removed in c2a7640.

Remove override as suggest bitcoin#10550 (comment)

laanwj · 2017-06-08T18:36:53Z

src/coins.h

    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {
+        throw std::logic_error("CCoinsViewCache cursor iteration not supported.");
+    }


Thanks! Though I'm still getting one warning after this:

In file included from /home/.../bitcoin/src/rpc/rawtransaction.cpp:8: /home/.../bitcoin/src/coins.h:214:10: warning: 'BatchWrite' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); ^ /home/.../bitcoin/src/coins.h:186:10: note: overridden virtual function is here bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override;

Fixed 36a14a9

ryanofsky

Squashed 36a14a9 -> 3ff1fa8 (pr/coins-stale.8 -> pr/coins-stale.9)

ryanofsky · 2017-06-08T21:13:43Z

src/coins.h

    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+    CCoinsViewCursor* Cursor() const override {
+        throw std::logic_error("CCoinsViewCache cursor iteration not supported.");
+    }


Fixed 36a14a9

3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky) 24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky) Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861

…rsor() 3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky) 24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky) Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861

ryanofsky mentioned this pull request Jun 7, 2017

Make CCoinsViewCache::Cursor() return latest data #9306

Closed

sipa reviewed Jun 7, 2017

View reviewed changes

ryanofsky commented Jun 7, 2017

View reviewed changes

ryanofsky force-pushed the pr/coins-stale branch from c754fd1 to c9d5b10 Compare June 7, 2017 20:23

laanwj reviewed Jun 8, 2017

View reviewed changes

paveljanik reviewed Jun 8, 2017

View reviewed changes

ryanofsky force-pushed the pr/coins-stale branch from c9d5b10 to b4ff57a Compare June 8, 2017 11:48

ryanofsky commented Jun 8, 2017

View reviewed changes

ryanofsky changed the title ~~Don't return stale data from CCCoinsViewCache::Cursor()~~ Don't return stale data from CCoinsViewCache::Cursor() Jun 8, 2017

Use override keyword on CCoinsView overrides

3ff1fa8

ryanofsky force-pushed the pr/coins-stale branch from b4ff57a to c2a7640 Compare June 8, 2017 13:54

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 8, 2017

fixup! Don't return stale data from CCCoinsViewCache::Cursor()

c2a7640

Remove override as suggest bitcoin#10550 (comment)

ryanofsky force-pushed the pr/coins-stale branch from c2a7640 to a51ff25 Compare June 8, 2017 13:55

laanwj reviewed Jun 8, 2017

View reviewed changes

ryanofsky force-pushed the pr/coins-stale branch from a51ff25 to 3ff1fa8 Compare June 8, 2017 21:14

ryanofsky commented Jun 8, 2017

View reviewed changes

laanwj merged commit 3ff1fa8 into bitcoin:master Jun 12, 2017

laanwj mentioned this pull request Jun 21, 2017

Use the override specifier (C++11) where we expect to be overriding the virtual function of a base class #10631

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ryanofsky commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 8, 2017

Uh oh!

laanwj Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ryanofsky commented Jun 7, 2017 •

edited

Loading

laanwj Jun 8, 2017 •

edited

Loading

laanwj commented Jun 8, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading