Instead of blocking, give 503?

It's not really unavailable, just that the background process needs to catch up, which should happen quickly.

Should we not check the return of BlockUntilSyncedToCurrentChain and maybe always return an error before its even gotten close to caught up?

Still needs to be addressed.

#11857 (comment)

Comment incomplete or outdated?

Thanks, fixed.

Move after Init?

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

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.

Separating these if statements loses coherency - if we crash we may have a bogus txindex and no way to detect it. Should likely instead do if (batch_newdb.SizeEstimate() + batch_olddb.SizeEstimate() > batch_size)........

Good catch.

In commit "[db] Create separate database for txindex."

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

Done.

In commit "[db] Create separate database for txindex."

Maybe declare const and add comment (16MiB).

Done.

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.

I think you are right that this should never happen, so I opted to return an error instead of logging and completing the migration.

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:

for (pcursor->Seek(begin_key);; pcursor->Next()) {
    if (!pcursor->Valid() || batch_newdb.SizeEstimate()...) {
        ...flush and compact...
    }
    if (!pcursor->Valid()) break;
    ...move entry at cursor...
}

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 (comment)

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:

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

How do you feel about this approach?

#11857 (comment)

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.

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.

In commit "[db] Create separate database for txindex."

#11857 (comment)

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 "[index] Create new TxIndex class."

Could const auto& to avoid a copy.

Done

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:

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

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 "[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.

Yeah, that wasn't even used. Left over from a previous version where the database wasn't split out.

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.

This isn't checking against chainActive, it is just asserting that BlockConnected gets called with blocks in order (even if chainActive is ahead). It's important to note that if the BlockConnected callbacks are running, then m_synced is true an ThreadSync has exited. I added more comments to these fields/methods in the header file to hopefully make that more clear.

Should this have a distinct error code, rather than RPC_INVALID_ADDRESS_OR_KEY? If I understand correctly, an RPC consumer should wait a little and try again if this happens.

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.

A distinct error code may make sense, though no opinion on what. Further, I'm not sure we should let GetTransaction call into the g_txindex if !f_txindex_ready (as it may return data from a stale block, but its also just generally not so useful). Maybe GetTransaction/FindTx can check if we're in sync yet.

Yeah, it's best effort, just like the lookup from CCoinsCacheView. If there's a response, great, otherwise the error message says that the not found may be inclusive. I agree that's kind of a weird behavior for the RPC, but if we change it, I could make the same argument for just removing the CCoinsCacheView lookup.

If it really takes 103 minutes on a fast EC2 instance, maybe make it 1%? Or once a minute? QT already shows 1% intervals during the upgrade.

The percentage shown in the UI is updated with every percent, it's just the log line that shows increments of 10, as it appends to the same line: [0%]...[10%]...[20%...]. Alternatively, it could log progress on a separate line every 30 seconds or something like: "Upgrading txindex database: n% complete\n". I kind of prefer that approach, but I copied how it was done for CCoinsViewDB upgrade.

I opted to log every 30s.

Can we get a better name here?

Suggest one.

Writing a batch deleting the thing the iterator is pointing to before calling Next() just feels broken to me. It should be fine cause iterators appear to always take a snapshot, but could you at least add a comment noting that we rely on this being safe (and also do in CCoinsViewDB::Upgrade).

Seems like overkill to me to leave a comment for documented LevelDB behavior, but sure.

I dont think this is right - if Read fails due to a DB error it will throw a dbwrapper_error, so we'll never get here anyway. It also makes the return value super confusing. Can we just make it have return semantics the same as every other DB function?

I'll change it, but I think the return value of CDBWrapper::Read() is super confusing on it's own. It means either that the key was not found or that the value at that key could not be deserialized into the provided struct (and that the return value is now in an unspecified state).

It'd be nice to check that the genesis block matches (if locator was non-null) as the locator will always have it. Not a big deal since users shouldnt be moving the txinded db to anothter datadir, but...users love to do crazy things and break things.

Eh, what's the right behavior in that case? Seems to me like it would be to sync the txindex from genesis, which is exactly what this does.

It'd be really nice to write the locator out at various points during this loop, as otherwise if you start a txindex sync and then kill your bitcoind while it takes its hour building the txindex you'll have to start against from scratch on next startup.

Good point, done.

Still needs to be addressed.