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

Index for BIP 157 block filters #14121

Merged
merged 12 commits into from Apr 18, 2019
@@ -41,9 +41,9 @@ bool BaseIndex::DB::ReadBestBlock(CBlockLocator& locator) const
return success;
}

bool BaseIndex::DB::WriteBestBlock(const CBlockLocator& locator)
void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator)
{
return Write(DB_BEST_BLOCK, locator);
batch.Write(DB_BEST_BLOCK, locator);
}

BaseIndex::~BaseIndex()
@@ -95,17 +95,22 @@ void BaseIndex::ThreadSync()
int64_t last_locator_write_time = 0;
while (true) {
if (m_interrupt) {
WriteBestBlock(pindex);
m_best_block_index = pindex;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 28, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (e3845e4)

It seems good to set m_best_block_index here, but I don't understand why it wasn't being set before. Is this a bugfix?

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 5, 2019

Author Contributor

Previously, since m_synced wasn't set to and WriteBestBlock took an explicit pointer argument, it wasn't necessary. But now WriteBestBlock reads m_best_block_index instead of taking an argument.

// No need to handle errors in Commit. If it fails, the error will be already be
// logged. The best way to recover is to continue, as index cannot be corrupted by

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 10, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (4368384)

I don't understand "index cannot be corrupted by a missed commit." Does it mean that if this Commit call fails you expect a future Commit call to fix whatever the problem is? If so, this seems like a property that holds for TxIndex, but not necessarily for BlockFilterIndex since that index is writing external files. Should clarify whatever is meant by this comment.

It would also be really helpful to give a specific example where this behavior would be desirable. Is there a case you are imagining where some commits fail and some succeed and after that everything is fine? I am having trouble trying to conjure up a scenario.

Naively, as a user relying on the index I would expect that if updating the index ever failed, then the index would get disabled and lookups into the index would return RPC errors instead of silently returning bad data or incomplete data. I would probably also want writing to stop after the first error, so I could potentially debug the issue, and so my logs wouldn't fill up with more and more write errors after the first one.

Maybe this implementation makes more sense than what I'm describing and is better for some reason, but whatever the rationale is, it could definitely be stated more clearly.

// a missed commit to disk for an advanced index state.
Commit();
return;
}

{
LOCK(cs_main);
const CBlockIndex* pindex_next = NextSyncBlock(pindex);
if (!pindex_next) {
WriteBestBlock(pindex);
m_best_block_index = pindex;
m_synced = true;
// No need to handle errors in Commit. See rationale above.
Commit();
break;
}
pindex = pindex_next;
@@ -119,8 +124,10 @@ void BaseIndex::ThreadSync()
}

if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
WriteBestBlock(pindex);
m_best_block_index = pindex;
last_locator_write_time = current_time;
// No need to handle errors in Commit. See rationale above.
Commit();
}

CBlock block;
@@ -144,15 +151,22 @@ void BaseIndex::ThreadSync()
}
}

bool BaseIndex::WriteBestBlock(const CBlockIndex* block_index)
bool BaseIndex::Commit()
{
LOCK(cs_main);
if (!GetDB().WriteBestBlock(chainActive.GetLocator(block_index))) {
return error("%s: Failed to write locator to disk", __func__);
CDBBatch batch(GetDB());
if (!CommitInternal(batch) || !GetDB().WriteBatch(batch)) {
return error("%s: Failed to commit latest %s state", __func__, GetName());
}
return true;
}

bool BaseIndex::CommitInternal(CDBBatch& batch)
{
LOCK(cs_main);
GetDB().WriteBestBlock(batch, chainActive.GetLocator(m_best_block_index));
return true;
}

void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex,
const std::vector<CTransactionRef>& txn_conflicted)
{
@@ -224,9 +238,10 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator)
return;
}

if (!GetDB().WriteBestBlock(locator)) {
error("%s: Failed to write locator to disk", __func__);
}
// No need to handle errors in Commit. If it fails, the error will be already be logged. The
// best way to recover is to continue, as index cannot be corrupted by a missed commit to disk
// for an advanced index state.
Commit();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 25, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (e3845e4)

It would be good to check for failures on Commit() calls in this git commit. Even though the current implementation in this git commit always returns true, it can start to return false when the subclasses is added, and these calls don't appear to be updated later.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 28, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (e3845e4)

Would be good to add NODISCARD to all methods that return errors on failure. Even in cases where you don't want to handle or log the errors, it's clarifying if invoking code shows that errors are being ignored, and maybe has comments saying why the errors are expected or ok to ignore.

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 5, 2019

Author Contributor

I explicitly don't want to handle failure as Commit() already logs on errors and in the places where the return value is ignored, the index is committing a later state so it is safe to just continue. I will add comments in the appropriate places saying this.

}

bool BaseIndex::BlockUntilSyncedToCurrentChain()
@@ -32,7 +32,7 @@ class BaseIndex : public CValidationInterface
bool ReadBestBlock(CBlockLocator& locator) const;

/// Write block locator of the chain that the txindex is in sync with.
bool WriteBestBlock(const CBlockLocator& locator);
void WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator);

This comment has been minimized.

Copy link
@lucayepa

lucayepa Mar 24, 2019

Contributor

ultranit, you can change the comment: txindex -> index.

};

private:
@@ -54,8 +54,15 @@ class BaseIndex : public CValidationInterface
/// over and the sync thread exits.
void ThreadSync();

/// Write the current chain block locator to the DB.
bool WriteBestBlock(const CBlockIndex* block_index);
/// Write the current index state (eg. chain block locator and subclass-specific items) to disk.
///
/// Recommendations for error handling:
/// If called on a successor of the previous committed best block in the index, the index can

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 10, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (4368384)

Thank you for adding this comment, but I had to read this several times before I figured out that the "index can continue processing" isn't describing something that happens externally but is describing what you are supposed to do when handling errors from this function. I'd suggest phrasing this less passively as:

/// Recommendation for handling errors returned by this function:
///
/// If calling this function fails, and m_best_block_index is a descendant
/// of a block that was previously committed, it is safe to ignore the error
/// because the index will not get corrupted (just needs to catch up from
/// further behind on reboot). If m_best_block_index is not a descendant of
/// the last block committed (due to a chain reorganization), the error can't
/// be ignored and the index should halt until Commit succeeds or it could 
/// end up getting corrupted.
/// continue processing without risk of corruption, though the index state will need to catch up
/// from further behind on reboot. If the new state is not a successor of the previous state (due
/// to a chain reorganization), the index must halt until Commit succeeds or else it could end up
/// getting corrupted.
bool Commit();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 28, 2019

Contributor

In commit "index: Allow atomic commits of index state to be extended." (e3845e4):

Giving the private method the same name as the protected method inherited by subclasses makes the code harder to follow. Would suggest calling this InternalCommit() or something like that.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 10, 2019

Contributor

re: #14121 (comment)

In commit "index: Allow atomic commits of index state to be extended." (4368384)

Current change isn't what I was suggesting and I think is actually worse than the original (but still ok if this is what makes most sense to you). I was suggesting renaming the Commit() that's internal to the base class InternalCommit(), not calling the other Commit() that's shared and overridden externally by subclasses InternalCommit(). Calling the overridden Commit() internal also breaks consistency with the other overridden methods (init, rewind, writeblock).

This comment has been minimized.

Copy link
@jimpo

jimpo Apr 10, 2019

Author Contributor

The difference from Init, Rewind, and WriteBlock and is that CommitInternal has a different method signature from Commit, which is why I thought you found it confusing, whereas the others have only a single method signature that the child classes override.

I don't have too much of a preference, though I do think switching the Internal would be weird, as I've generally found the convention to be that an outer function delegating to an inner helper function might call FuncInternal, not the other way around.


protected:
void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex,
@@ -69,6 +76,10 @@ class BaseIndex : public CValidationInterface
/// Write update index entries for a newly connected block.
virtual bool WriteBlock(const CBlock& block, const CBlockIndex* pindex) { return true; }

/// Virtual method called internally by Commit that can be overridden to atomically
/// commit more index state.
virtual bool CommitInternal(CDBBatch& batch);

virtual DB& GetDB() const = 0;

/// Get the name of the index for display in logs.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.