💬 jesterhodl commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2249392438)
transifex updated

💬 hebasto commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3146725108)
@theStack

Can you confirm the bug?

🤔 hodlinator reviewed a pull request: "index: store per-block transaction locations for efficient lookups"
(https://github.com/bitcoin/bitcoin/pull/32541#pullrequestreview-3081181690)
Concept ACK 1b928f58fc78f4727cef988902075abc05c372b2

Cool way of compressing transaction metadata to significantly reduce on-disk footprint compared to `TxIndex` and make it more likely this one fits in RAM. (Fine as long as external code can fetch by block hash + tx index instead of by txid).

Optimization as I understand it: All transactions within the same block have the same `FlatFilePos` base, which is compressed by a factor of 128 thanks to `TXS_PER_ROW`. Instead of storing an individ
...

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249347002)
Unused variable. Was this supposed to be used to skip forward in the stream instead of deserializing the header?

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249356202)
nit:
```suggestion
return RESTERR(req, HTTP_NOT_FOUND, strprintf("Failed to read transaction #%d from block %s", index, block_hash->ToString()));
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249355515)
We can skip this work if `LocationsIndex` is enabled as it doesn't use `block_pos`.

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249370232)
nit: Might be less confusing to use "row" everywhere here instead of "part"?

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249371490)
nit: Should use snake_case naming convention as per developer-notes.md, `n_tx`/`num_tx`/`tx_count`?

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249348139)
Would prefer it be moved to function-local scope so we don't add any startup time for those who never enable -locationsindex or use the REST interface.

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249374257)
nit: Unused header.

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249370654)
nit: Could use local variable with descriptive name rather than mutating in-param:
```suggestion
const auto column = i % TXS_PER_ROW; // index within a single DB row
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2249349229)
nit:
* Improved order?
* Clarify that wasted work is being done.
```suggestion
By default, this endpoint will also deserialize the leading transactions, before reading and returning the requested one.
```

💬 luke-jr commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2249411442)
same

💬 luke-jr commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2249411415)
This should come (ultimately) from /CMakeLists.txt, not have to be re-specified here

💬 luke-jr commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249412471)
Adding command line options is non-trivial for GUI users. We should use the existing "data corrupt" message/prompt, so they can click a button to reindex.

💬 luke-jr commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249412210)
Should probably just replace the assert

💬 Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249504606)
The alert doesn't terminate the app. We need the assert there for termination.

💬 Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249504661)
The alert doesn't terminate the app. We need the assert there for termination.

💬 Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249504887)
We need to terminate the app after the alert. Keeping it going to lead to unsafe behaviour given that the data is corrupt.

💬 Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#discussion_r2249505556)
I agree this is a bad experience for non technical users.

But it seems like this is the standard right now in the code, to give this kind of alert on the code base (ie tell the the user to reindex instead of a direct action button)

If there is somewhere that actually has a button that reindexes, show me, I'll use it. If not, adding this functionality is work for another pull request.