rest: don't log an error for /spenttxouts on a block without undo data #36619

kriss39 commented at 9:21 AM on October 23, 2026: contributor

/rest/spenttxouts/<hash> reads the undo file for every block above genesis. For a block that was stored but never connected (a side branch) or whose undo data was pruned, there is no undo position, so OpenUndoFile() fails and ReadBlockUndo() writes OpenUndoFile failed for FlatFilePos(nFile=-1, nPos=0) to debug.log with LogError. The request itself already returns 404, but each unauthenticated request for such a block adds an [error] line that looks like disk corruption.

This checks CheckBlockDataAvailability() for BLOCK_HAVE_UNDO before reading, so nothing is logged. Pruning clears BLOCK_HAVE_UNDO, so pruned blocks are covered too. The response is unchanged.

The new check at the end of interface_rest.py requests a stale block and fails on master because of the logged error.

DrahtBot added the label RPC/REST/ZMQ on Oct 23, 2026

DrahtBot commented at 9:21 AM on October 23, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36619
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36619.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36619#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in src/rest.cpp:377 in be3c1db50f

 371 | @@ -372,8 +372,12 @@ static bool rest_spent_txouts(const std::any& context, HTTPRequest* req, const s
 372 |      }
 373 |  
 374 |      CBlockUndo block_undo;
 375 | -    if (pblockindex->nHeight > 0 && !chainman->m_blockman.ReadBlockUndo(block_undo, *pblockindex)) {
 376 | -        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " undo not available");
 377 | +    if (pblockindex->nHeight > 0) {
 378 | +        // Blocks that were never connected or have been pruned have no undo data.
 379 | +        const bool have_undo{WITH_LOCK(cs_main, return (pblockindex->nStatus & BLOCK_HAVE_UNDO) && !chainman->m_blockman.IsBlockPruned(*pblockindex))};

stickies-v commented at 10:13 AM on October 23, 2026:

I think the && !chainman->m_blockman.IsBlockPruned(*pblockindex) check here is redundant?

maflcko commented at 10:39 AM on October 23, 2026:

Can this use CheckBlockDataAvailability to avoid duplicating the impl?

kriss39 commented at 10:59 AM on October 23, 2026:

Thanks, both right. PruneOneBlockFile() clears BLOCK_HAVE_UNDO, so the IsBlockPruned() check added nothing. It now uses CheckBlockDataAvailability(*pblockindex, *pblockindex, BlockStatus{BLOCK_HAVE_UNDO}), in e8af2c7488925e35ffeaf64e523cef48b70280eb.

stickies-v commented at 11:33 AM on October 23, 2026:

I think a simple pblockindex->nStatus & BLOCK_HAVE_UNDO is more straightforward tbh but either way works.

maflcko commented at 2:56 PM on October 23, 2026:

yeah, i was thinking that the rpc CheckBlockDataAvailability would provide better error messages, but if they are not used, a plain nStatus & BLOCK_HAVE_UNDO seems fine.

stickies-v commented at 10:17 AM on October 23, 2026: contributor

Approach ACK

rest: don't log an error for /spenttxouts on a block without undo data

`/rest/spenttxouts/<hash>` calls `ReadBlockUndo()` for every block above
genesis. For a block that was stored but never connected (a side branch)
or whose undo data was pruned, `GetUndoPos()` is null, so
`OpenUndoFile()` fails and `ReadBlockUndo()` writes "OpenUndoFile failed
for FlatFilePos(nFile=-1, nPos=0)" to debug.log with `LogError`. The
endpoint then correctly answers 404, but every unauthenticated request
for such a block adds an `[error]` line that looks like disk corruption.

Check `CheckBlockDataAvailability()` for `BLOCK_HAVE_UNDO` first and
only read the undo file when it can exist. Pruning clears
`BLOCK_HAVE_UNDO`, so this covers pruned blocks too. The response is
unchanged.

The new interface_rest.py check requests a side-branch block and fails
on master because of the logged error.

e8af2c7488

kriss39 force-pushed on Oct 23, 2026

in src/rest.cpp:377 in e8af2c7488

 371 | @@ -372,8 +372,12 @@ static bool rest_spent_txouts(const std::any& context, HTTPRequest* req, const s
 372 |      }
 373 |  
 374 |      CBlockUndo block_undo;
 375 | -    if (pblockindex->nHeight > 0 && !chainman->m_blockman.ReadBlockUndo(block_undo, *pblockindex)) {
 376 | -        return RESTERR(req, HTTP_NOT_FOUND, hashStr + " undo not available");
 377 | +    if (pblockindex->nHeight > 0) {
 378 | +        // Blocks that were never connected or have been pruned have no undo data.
 379 | +        const bool have_undo{WITH_LOCK(cs_main, return chainman->m_blockman.CheckBlockDataAvailability(*pblockindex, *pblockindex, BlockStatus{BLOCK_HAVE_UNDO}))};

maflcko commented at 11:13 AM on October 23, 2026:

heh, that's funny. I was talking about src/rpc/blockchain.h:void CheckBlockDataAvailability(node::BlockManager& blockman, const CBlockIndex& blockindex, bool check_for_undo) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);, but I guess this works as well.

edit: I was thinking about CheckBlockDataAvailability from the rpc, to have more accurate error messages, but maybe that is not too important for the rest interface.

maflcko approved

maflcko commented at 11:13 AM on October 23, 2026: member

lgtm ACK e8af2c7488925e35ffeaf64e523cef48b70280eb

stickies-v commented at 11:33 AM on October 23, 2026: contributor

ACK e8af2c7488925e35ffeaf64e523cef48b70280eb