Build tx index in parallel with validation #11857

In commit “[index] Create new TxIndex class.”

No, this has to be registered before m_synced is set to true in Init.

Definitely worth noting this in a code comment. Also maybe m_synced should be called m_initialized or m_started to be clearer that it changes from false to true just once on startup, and isn’t updated in an ongoing way.

In commit “[db] Create separate database for txindex.”

Maybe change to const auto& tuple to avoid a copy while iterating.

In commit “[db] Create separate database for txindex.”

Maybe declare const and add comment (16MiB).

In commit “[db] Create separate database for txindex.”

Maybe log an error in this case. I don’t think it would be expected for pcursor->Valid() to return true but pcursor->GetKey to fail.

In commit “[db] Create separate database for txindex.”

This is duplicating code inside the for loop, and also skipping the last compact range on the old db. Maybe just flush and compact once at the top of the loop:

0for (pcursor->Seek(begin_key);; pcursor->Next()) {
1    if (!pcursor->Valid() || batch_newdb.SizeEstimate()...) {
2        ...flush and compact...
3    }
4    if (!pcursor->Valid()) break;
5    ...move entry at cursor...
6}

Hmm, the logic gets kind of tricky since there’s also the early break if the cursor is valid but iterates past the DB_TXINDEX range. Also the CompactRange after the loop compacts over the entire range, not just from the previous batch write point. What do you mean that it skips the last compact range on the old DB? It shouldn’t.

This code was mostly copied from CCoinsViewDB::Upgrade if that wasn’t clear.

#11857 (review)

In commit “[db] Create separate database for txindex.”

Hmm, the logic gets kind of tricky since there’s also the early break.

This doesn’t seem that bad in light of the code duplication in the current version of this code, and the fact that the originally duplicated code had a bug that needed to be fixed two places instead of one (missing the “Sync new DB changes to disk before deleting from old DB” behavior).

I think adding new done bool variable set to as !pcursor->Valid() || key.first != DB_TXINDEX, would make the code change I suggested above even shorter and clearer.

What do you mean that it skips the last compact range on the old DB? It shouldn’t.

My mistake, I don’t think I saw the actual line of code I was commenting on.

I have tried a few things, and the approach you describe is not worth the additional control flow complexity IMO. Consider also that the CompactRange calls are over a different range inside and outside of the loop (as noted above) and the newest version of the code adds an additional write to the batch outside of the loop to write the block hash.

However, I understand the argument against duplication, so I instead added a private method extracting the logic:

0static void WriteTxIndexMigrationBatches(TxIndexDB& newdb, CBlockTreeDB& olddb,
1                                         CDBBatch& batch_newdb, CDBBatch& batch_olddb,
2                                         const std::pair<unsigned char, uint256>& begin_key,
3                                         const std::pair<unsigned char, uint256>& end_key)
4{
5    ...
6}

How do you feel about this approach?

#11857 (review)

How do you feel about this approach?

Better, I’m still not sure why my suggestion doesn’t work out, but this takes care of my concern.

In commit “[db] Create separate database for txindex.”

Unclear why migration should be tied to txindex flag. At first glance, it would seem simpler to reason about possible states and also more robust if the code just always moved DB_TXINDEX entries from the old location to the new location independent of the flag. Maybe add a code comment here to clarify rationale.

In commit “[db] Create separate database for txindex.”

#11857 (review)

Unclear why migration should be tied to txindex flag. At first glance, it would seem simpler to reason about possible states and also more robust if the code just always moved DB_TXINDEX entries from the old location to the new location independent of the flag. Maybe add a code comment here to clarify rationale.

I elaborated on the comment. I think this is the best way to determine whether a migration is necessary. LMK if you think it needs further explanation.

This still doesn’t seem to be saying why the flag needs to be checked. Maybe add “Do not migrate data if the txindex flag is false, because any data that is found will be stale and not in sync with DB_BEST_BLOCK” to the comment if that’s the explanation.

In commit “[db] Methods on TxIndexDB to persist best block hash. "

Is this a mistake? Probably shouldn’t be adding an unused parameter to an unrelated method in this commit.

#11857 (review)

In commit “[db] Methods on TxIndexDB to persist best block hash. "

It does get used.

My mistake. IMO, it would make review a little easier if this commit were squashed into the previous commit. I don’t think there’s benefit to adding the new TxIndexDB class in one commit, and then modifying it to add this simple feature separately.

In commit “[index] Create new TxIndex class.”

Maybe restructure the code to get rid of this early successful return. It’s a little confusing, and also not clear if it’s 100% correct. For example I would think that if best_block_hash and chain_tip are both null, m_synced should still be set to true below.

In commit “[index] Create new TxIndex class.”

Could const auto& to avoid a copy.

In commit “[index] Create new TxIndex class.”

Consider moving WriteBestBlockHash call out to caller so the stored best block and m_best_block_index get updated together. It’s a little unexpected to see one value written without the other. Might want to rename this method to WriteBlockTxns if you do this.

I kind of prefer to keep the DB writes together. I’d hoped that the access pattern would make it clear:

0if (WriteBlock(*block, pindex)) {
1    m_best_block_index = pindex;
2}

In commit “[index] Create new TxIndex class.”

Why use a different code for DB_TXINDEX_BEST_BLOCK than DB_BEST_BLOCK? Why even define a new constant at all? It seems strange that the new txindex format would diverge unnecessarily from the old format here when it isn’t doing do that in other places. Should add a code comment explaining if there is a reason.

In commit “[index] Create new TxIndex class.”

I’m not sure this case and the one above should be errors given that BlockConnected calls are queued up in the notification thread and may not be up to date with chainActive (which is what ThreadSync syncs toward). Seems like it would be right (and simpler) to just call WriteBlock unconditionally here.

Yes, that’s right, an RPC consumer should wait until the index is built. Using the RPC_INVALID_ADDRESS_OR_KEY error code for the "No such mempool transaction. Use -txindex to enable blockchain transaction queries" clause also seems odd to me.

Perhaps the RPC_IN_WARMUP makes sense here, or perhaps a new RPC_INDEX_UNAVAILABLE error code.

This is a lot of moves, since you already move TxIndex::TxIndex. Can you change to create the unique_ptr in the constructor call, e.g.:

0g_txindex.reset(new TxIndex(MakeUnique<...>(....));

Or even better, use argument forwarding.

I’d rather have them on separate lines because I think otherwise there is too much happening on one line (and it would get pretty long).

Regarding argument forwarding, even if TxIndex takes an rvalue-ref, the std::move is necessary.

Can you make this forwardable?

0TxIndex(std::unique_ptr<TxIndexDB> &&db);

Or just create the unique_ptr here, see my other comment regarding g_txindex.reset()

That’s not strictly accurate:

 0$ g++ --version
 1g++ (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
 2Copyright (C) 2017 Free Software Foundation, Inc.
 3This is free software; see the source for copying conditions.  There is NO
 4warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 5
 6$ cat reserve.cc 
 7#include <iostream>
 8#include <vector>
 9
10struct Foo {
11    int x;
12    Foo() :x(1) {}
13};
14
15int main(int argc, char **argv) {
16    std::vector<Foo> foovec;
17    foovec.reserve(10);
18    std::cout << foovec[5].x << std::endl;
19    std::cout << foovec.at(5).x << std::endl;
20    return 0;
21}
22
23$ g++ reserve.cc 
24
25$ ./a.out 
260
27terminate called after throwing an instance of 'std::out_of_range'
28  what():  vector::_M_range_check: __n (which is 5) >= this->size() (which is 0)
29Aborted (core dumped)

In this test Foo has a constructor that sets x to 1, but as you can see from the first line of output it prints 0 implying that the ctor wasn’t actually called. The .at() check fails because the element is out of bounds compared to the true size. The standard calls this “default insertable” which I think just means “not allocator aware” in which case the default allocator is allowed to just create space for the item.

Either way it’s a nit that you can ignore, but I think the suggestion I made is safe and slightly faster, assuming you actually initialize the elements before accessing them.

The transaction logic throughout is wrong here because there’s a data ordering dependency.

During the data migration:

• Write to new new with fsync = True
• After new write finishes, you can erase from old db (fsync setting can be left false)

This ensures that you can can always scan through the old database and catch up to what’s in the new database if there’s a crash or you are interrupted. During the non-migration path I think it’s fine to have fsync disabled.

In commit “[db] Create separate database for txindex.”

Commit title could be changed to something like “Add (unused) TxIndexDB class” to be more specific. Current title is a little misleading about what the actual change is.

In commit “[db] Create separate database for txindex.”

I think it’d be better to disable copying in CDBWrapper instead of here, since the state that’s actually unsafe to copy is in that class, not this one.

In commit “[db] Create separate database for txindex.”

Could say cursor instead of pcursor to avoid more hungarian names.

In commit “[index] Create new TxIndex class.”

Not really the “block index database” now, but a separate database (unless I’m misinterpreting)?

In commit “[index] Create new TxIndex class.”

Maybe set to this false here to simplify initialization in the constructor. Similarly for atomic member below.

In commit “[index] Create new TxIndex class.”

Maybe add comment about initialization sequence given the three initialization methods in this class (constructor, Init, and Start).

In commit “[index] Create new TxIndex class.”

It seems hungarian notation can never go away even in new code, given how sticky the pindex convention seems to be, but maybe consider calling this block or best_block_index instead.

In commit “[index] Create new TxIndex class.”

It seems needlessly inefficient and complicated to be creating a vector just to be able to pass a sequence of txids to the TxIndexDB::WriteTxs method, which is only called from this one place. It seems like a simpler design might inline the WriteTx method, or pass a block to it, or just not have separate TxIndexDB and TxIndex classes.

#11857 (review)

Yes, it could be more efficient, but I feel that this produces the cleanest separation of responsibilities.

I’m not sure what the separation of responsibilities is actually. If it’s important to avoid a more direct write, can you say somewhere in a comment what the separation is supposed to be, and what benefits it provides? This just seems a little more complicated than I would expect it to be.

In commit “[index] Create new TxIndex class.”

Would expand this comment to mention what could actually trigger these errors. Maybe add “If the errors below are triggered, it means a BlockConnected call has been received for a new block which has ancestor blocks that might never have been added to the transaction index. This could happen if a bug causes BlockConnected notifications to be sent in the wrong order, or if there’s an unfortunately timed reorg or invalidateblock call that causes the first BlockConnection notification after shutdown of the initial sync thread to not be an ancestor or direct descendant of chainActive at the time the sync completed.”

#11857 (review)

I’ll elaborate on the comment, but I don’t understand the case you are describing

The BlockConnected handler will drop any notifications as long as m_synced is false, but when m_synced becomes true, there is no code to clear out the queue, which could be backlogged. So if m_synced is set to true after a reorg but before BlockConnected calls from the reorg arrive, some initial BlockConnected notifications about blocks that aren’t descendants of the synced best block could fail.

In commit “[index] TxIndex initial sync thread.”

Could declare static to avoid creating a linker symbol.

Correct me if I’m wrong, but I assume this could be inlined or something by the compiler if it were not declared static. I tried looking it up briefly and it said something along those lines, but then I got bored because I don’t care all that much (something, something ODR-rule?). Will change if you still want.

Also, see #11857 (review).

#11857 (review)

Correct me if I’m wrong, but I assume this could be inlined or something by the compiler if it were not declared static. I tried looking it up briefly and it said something along those lines, but then I got bored because I don’t care all that much (something, something ODR-rule?). Will change if you still want.

Hmm, I guess you and @eklitzke are making me question my sanity, but why would adding static here prevent inlining, or “reserve .bss space” or do anything along these lines?

If this isn’t declared static, the compiler doesn’t know that there isn’t some code in another object file that will access the SYNC_LOG_INTERVAL variable or take its address. So there will necessarily be a SYNC_LOG_INTERVAL symbol in the object file. If it’s static, it can just be internal to the object.

In commit “[init] Initialize and start TxIndex in init code.”

Might be more consistent to use MakeUnique here too.

In commit “[init] Initialize and start TxIndex in init code.”

Could combine lines and shorten by just passing nTotalCache / 8 directly to min. Same for nBlockTreeDBCache above.

In commit “[init] Initialize and start TxIndex in init code.”

What’s the reason for halving this even if txindex is disabled?

[index] TxIndex method to wait until caught up.

Description above and function name seem a little misleading since this will only wait to be caught up with validation notifications, and will actually return false immediately during startup. This is probably worth mentioning in the comment above, and maybe in an additional comment where this is called. Could also give this this a less reassuring name like FinishProcessingCurrentBlock.

In commit “[test] Simple unit test for TxIndex.”

Would be good to validate postx variable contents here and below.

In commit “[test] Simple unit test for TxIndex.”

Would suggest adding a few more simple checks:

• Test that sync thread actually works. I.e. that if new transactions are added before calling Start(), they don’t show up, and that they do show up after calling Start().
• Check that transactions are persistent and reloaded correctly if txindex is closed and reopened.
• Check BlockUntilSyncedToCurrentChain return value. Should be false before started, true after.

I added checks for 1 & 3. With regards to persistence, the DB in the test environment currently uses the in-memory LevelDB environment rather than actually writing to disk, which makes cleanup nicer and probably makes the test marginally faster.

How important do you think it is to test the persistence at this layer? My feeling is that it’s probably not adding much since it’s just using the common CDBWrapper abstraction.

This check is too strict for the BlockConnected validationinterface callback - you really want some variant of the SetBestChain validationinterface callback (which is called when new blocks/header tree is flushed to disik). Practically, I’m not sure what the best way to sovle this is:

• You could move txindex stuff to a cache ala pcoinsTip and then flush it on some SetBestChain variant (which would need to be tweaked to not have its own timer, which I think we should do anyway, and use the fPeriodicWrite bool or so), but then you’d need to plumb the txindex cache usage back into the FlushStateToDisk flush condition. I think this is probably the easiest approach, but people will probably not be too happy with the circular dependency here. You could get around that with some higher-level “total memory usage tracking” interface, but scope creep….
• You could move to storing a locator in the txindex DB like the wallet does and then reconnect from that on startup from whatever the locator can find. This has a super weird edge-case where you may have dangling entries pointing to blocks which were stale, which were connected, (ie “written” to disk) but then lost as they were not flushed to disk, but that should be rare enough the overhead of DB entries isnt a big deal, as long as you handle such failed reads appropriately.

A block may be connected, but the block index may not be written to disk until much later. Thus, we can run far ahead here. Specifically, we flush in the following order:

• Blocks are written to disk before connecting them (but not fsync’d, so not guarantees here!). At the same time entries are added to mapBlockIndex/CBlockIndex*s are created, but not flushed to disk)
• Blocks are validated/connected, generating BlockConnected callbacks.
• At some point in the future, flushing happens, where we first fsync the blocks written to disk, then write out the mapBlockIndex changes, then write out pcoinsTip/UTXO DB.

That’s what I did originally, but that caused problems when building tests. The test binary is not linked against init.cpp, it’s linked against test_bitcoin_main.cpp, which is why g_connman is also defined there. That took me an annoying amount of time to figure out, and by the time I did I was so pissed off I decided to move it to txindex.cpp.

This whole thing where stuff in init has to get redefined in test_bitcoin_main.cpp if it’s used in the tests seems pretty gross. Do you really thing that’s the right way to do it?

In commit “!fixup Migration to handle downgrades gracefully.”

I really like this DB_TXINDEX_BLOCK solution to the problem of recovering from an interrupted upgrade. It’s surprisingly simple.

Not sure if it’s worth the additional work, but it seems it’d be possible to write some simple unit tests that exercise this code by running it on databases with known states (with “txindex”, with both “txindex” and BLOCK, with BLOCK but no txindex, etc) and checking the results.

In commit “!fixup Change TxIndexDB to read/write locators instead of hashes.”

Would it make sense to assert locator.front() == m_best_block_index.GetBlockHash() as a sanity check on event ordering?

In commit “[index] Create new TxIndex class.”

It still seems to me like it would be possible (though unlikely) for these errors to trigger spuriously, even when the events arrive in the right order, but the notification queue gets backlogged: #11857 (review)

If this is true, the current comment seems misleading, and could mention the edge case.