💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301676036)
nit: Might as well use the fact that it's only 1 char:
```suggestion
strHex.push_back('\n');
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301710958)
2 nits:
1. Could reword and elaborate
```suggestion
* LocationsIndex is used to store and retrieve the location of transactions within a block.
*
* This is done in a compressed manner, allowing the whole index to fit in RAM on decent hardware.
* For example, the on-disk size was <X>GiB at block height <Y>.
```
2. Could clarify PR description unless I'm mistaken.
```diff
-allowing it to be cached
+allowing it to be cached in RAM
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2300933020)
nit: Could make `row` `const` as well and nail down the types:
```suggestion
bool LocationsIndex::ReadRawTransaction(const uint256& block_hash, uint32_t i, std::vector<std::byte>& out) const
{
const uint32_t row{i / TXS_PER_ROW}; // used to find the correct DB row
const uint32_t 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_r2301692085)
nit: Since we already have `index` and `locations_index` in scope here, could call this `block`, or `block_index`?
```suggestion
const CBlockIndex* block{chainman.m_blockman.LookupBlockIndex(*block_hash)};
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301697459)
nit: Should use snake_case as per developer-notes.md:
* `uri_parts`
* `str_hex`
* `obj_tx`
* `str_JSON`

💬 Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3227126023)
I tried if f474673fed789257b2a9e9e5aabe6e734128b00a fixes #32759. It doesn't, but that's probably not expected since it just removes an implicit dependency.

To update the time machine, I had to manually `guix download` one item to build with `--no-substitutes`:
- `guix download https://downloads.sourceforge.net/project/boost/boost/1.83.0/boost_1_83_0.tar.bz2`

Still in the process of updating the time machine x86_64 which is taking quite long. Will publish hashes when that's done and also
...

💬 hodlinator commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2303151367)
Why change the C++ Coding Style? I got used to the current one, which encourages `struct`s for POD as a separate thing.

📝 maflcko opened a pull request: "test: Use extra_port() helper in feature_bind_extra.py"
(https://github.com/bitcoin/bitcoin/pull/33260)
This is a refactor for self-validating and self-documenting code.

Currently, the test assumes that extra ports are available and just increments them without checking. However, this may not be the case when the test is modified to use more ports. In this case, the tests may fail intermittently and the failure is hard to debug.

Fix this confusion, by calling `p2p_port` each time. This ensures the required `assert n <= MAX_NODES` is checked each time.

Closes https://github.com/bitcoin/bit
...

🚀 fanquake merged a pull request: "[29.x] backport #33212"
(https://github.com/bitcoin/bitcoin/pull/33251)

📝 fanquake opened a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261)
`dash` is available again: https://bugzilla.redhat.com/show_bug.cgi?id=2335416.

💬 maflcko commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3227642428)
I can reproduce via `-DAPPEND_CXXFLAGS='-O1 -g2'`, but it passes via `-DAPPEND_CXXFLAGS='-O1 -fno-inline -g2'`, so I wonder if this is a compiler error, or if the error is something else.

👍 maflcko approved a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261#pullrequestreview-3159280776)
lgtm. Either shell should be fine here, for the purpose to test a non-bash shell.

💬 maflcko commented on pull request "ci: return to using dash in CentOS job":
(https://github.com/bitcoin/bitcoin/pull/33261#discussion_r2303567489)
nit: No need to advertise it here. This will only create a conflict with the GHA rewrite?

💬 maflcko commented on pull request "ci: return to using dash in CentOS job":
(https://github.com/bitcoin/bitcoin/pull/33261#discussion_r2303567806)
nit: remove ksh?

💬 maflcko commented on pull request "ci: return to using dash in CentOS job":
(https://github.com/bitcoin/bitcoin/pull/33261#issuecomment-3227827179)
lgtm ACK 509ffea40abbc706ef8b8fc449b7de8677fc5096

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3227873440)
The sidecar Template Provider application now has its own repo: https://github.com/Sjors/sv2-tp

I suspect that the SRI team will have a Rust alternative soon(tm) as well.

🤔 janb84 reviewed a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261#pullrequestreview-3160028763)
crACK 509ffea40abbc706ef8b8fc449b7de8677fc5096

PR Partially reverts faaabfaea768deb7767c489d32fd2097fd180872 because https://bugzilla.redhat.com/show_bug.cgi?id=2335416 is fixed.

- code-review ✅
- history-review ✅

💬 l0rinc commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2304364072)
We can define it that way as well (though I don't agree with the distinction vs classes), but not specifying it directly results in different versions formatting structs differently, so we should define it explicitly

💬 jsarenik commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-3228916881)
Let me note here that I noticed the zero (tested with LN Anchor) outputs are not being cleared in a read-only descriptor wallet even after they've been spent by a fee-paying transaction in a package.

💬 instagibbs commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-3228924450)
@jsarenik could you open a new issue?