-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Don't return stale data from CCoinsViewCache::Cursor() #10550
Conversation
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.
src/coins.h
Outdated
@@ -212,6 +212,9 @@ class CCoinsViewCache : public CCoinsViewBacked | |||
uint256 GetBestBlock() const; | |||
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in fe76b30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashed fe76b30 -> c9d5b10 (pr/coins-stale.2 -> pr/coins-stale.3)
src/coins.h
Outdated
@@ -212,6 +212,9 @@ class CCoinsViewCache : public CCoinsViewBacked | |||
uint256 GetBestBlock() const; | |||
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in fe76b30
utACK c9d5b10 |
src/coins.h
Outdated
@@ -212,6 +212,9 @@ class CCoinsViewCache : public CCoinsViewBacked | |||
uint256 GetBestBlock() const; | |||
void SetBestBlock(const uint256 &hashBlock); | |||
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); | |||
CCoinsViewCursor* Cursor() const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in c2a7640.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back overrides in a51ff25
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. |
@@ -482,6 +483,9 @@ bool ResetBlockFailureFlags(CBlockIndex *pindex); | |||
/** The currently-connected chain of blocks (protected by cs_main). */ | |||
extern CChain chainActive; | |||
|
|||
/** Global variable that points to the coins database (protected by cs_main) */ | |||
extern CCoinsViewDB *pcoinsdbview; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coins View DB vs coins db view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Thanks for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashed c2a7640 -> 24e44c3 (pr/coins-stale.4 -> pr/coins-stale.6)
src/coins.h
Outdated
@@ -212,6 +212,9 @@ class CCoinsViewCache : public CCoinsViewBacked | |||
uint256 GetBestBlock() const; | |||
void SetBestBlock(const uint256 &hashBlock); | |||
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); | |||
CCoinsViewCursor* Cursor() const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in c2a7640.
Remove override as suggest bitcoin#10550 (comment)
src/coins.h
Outdated
void SetBestBlock(const uint256 &hashBlock); | ||
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); | ||
CCoinsViewCursor* Cursor() const override { | ||
throw std::logic_error("CCoinsViewCache cursor iteration not supported."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 36a14a9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashed 36a14a9 -> 3ff1fa8 (pr/coins-stale.8 -> pr/coins-stale.9)
src/coins.h
Outdated
void SetBestBlock(const uint256 &hashBlock); | ||
bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); | ||
CCoinsViewCursor* Cursor() const override { | ||
throw std::logic_error("CCoinsViewCache cursor iteration not supported."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…rsor() 3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky) 24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky) Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861
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.