💬 instagibbs commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2323344088)
probably should be explicit in comments that this is a memory optimization, letting hole-having clusters be reconstructed without holes

💬 instagibbs commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2323362751)
this in unconditionally called I think

💬 instagibbs commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2323384935)
not sure I track why the command values are being mapped to specific series of actions. Writing it out in case it ends up helpful:

0:
a. AddTransaction if txns below set limit
b. AddDependency if any txns exist
c. RemoveTransactions if any txns exist
d. Compact
1:
a. AddTransaction if no transaction exists
b. AddDependency (transaction will exist)
c. RemoveTransactions (transaction will exist)
d. Compact

2:
a. AddTransaction if no transaction exists
b. RemoveTransactions (trans
...

🤔 mzumsande reviewed a pull request: "net: Quiet down logging when router doesn't support natpmp/pcp"
(https://github.com/bitcoin/bitcoin/pull/33311#pullrequestreview-3187161713)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814

💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3255622984)
Rebased and updated using the [latest version](https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/188) of the tool.

💬 TheCharlatan commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323536325)
How does this condition get fulfilled? I set `uses_wallet` manually in the `set_test_params`, but then the assertion on line 161 fails, which is a bit puzzling, because that would mean something changed in the template in the meantime.

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323550726)
It's set by `skip_if_no_wallet`.

💬 sipa commented on pull request "net: Quiet down logging when router doesn't support natpmp/pcp":
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255678817)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814

💬 davidgumberg commented on pull request "net: Quiet down logging when router doesn't support natpmp/pcp":
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255679309)
Tested ACK https://github.com/bitcoin/bitcoin/commit/4f1a4cbccd784e25f7932f1d0293602ef7f3e814

Ran this branch on a network where the default gateway does not support NatPMP, with `-natpmp=1` and `pcp: Could not receive response: Connection refused` is not printed, on master it is.

```console
$ mkdir -p /tmp/datadir && ./build/bin/bitcoind -regtest -natpmp=1 | grep -i pcp
```

On master:

```console
2025-09-04T21:03:19Z [net:warning] pcp: Could not receive response: Connection refu
...

💬 TheCharlatan commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323560338)
Right, so we might want to call that here?

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323573078)
Oh, I see.

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3255825130)
> I think that the performance gain will increase when fetching transactions near the end of the block.

Oof, I can confirm. Skipping over the serialization of all the prior transactions really helps. :D

<details><summary>Details</summary>

### 1 worker - 57x improvement

```shell
₿ hey -c 1 -n 10000 http://localhost:8332/rest/txfromblock/000000000000000000017bfd05b5fa367a424c4a565a4baf7950d9e8605df8ec-5000.bin
```

#### -locationsindex=0

```
Summary:
Total: 314.4807 secs

...

💬 davidgumberg commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#issuecomment-3255841202)
ACK https://github.com/bitcoin/bitcoin/pull/33278/commits/bad909272545defe9f7d8f27cfa3ff79859aa828

We should not be building functional tests without the minimum python version satisfied, it seems fine to leave it as a warning for maintenance and deploy targets, since these will error later when attempting to build the target.

💬 glozow commented on pull request "doc: archive v29.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255884530)
Added a couple of sentences to the description. I'm not going to address the review comments since this is a copy for master. In the future, these comments might be in scope on the backports and final changes PRs.

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3255907850)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

I'm still learning, but I think I did the squashing correctly now, hopefully! Thanks for your patience and hope I can be of some value to the project :)

👍 l0rinc approved a pull request: "headerssync: Correct unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-3187302261)
ACK eb0f41ee1ed9ea54019fcaa4e3ce33481da4459d

I have checked the diffs after rebase, regenerated and verified the mainchain params, looked for leftovers of decommissioned constants and went over the code again quickly to make sure I still agree with everything after the new changes.

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2323619897)
The new value after the rebase is:
```
- Initial: period=626, buffer=14854, mem=710.549 KiB
- New best: period=634, buffer=15061, mem=706.160 KiB
- New best: period=632, buffer=15009, mem=703.804 KiB

Given current min chainwork headers of 912683, the optimal parameters for low
memory usage on mainchain for release until 2028-04-02 is:

// Generated by headerssync-params.py on 2025-09-04.
m_headers_sync_params = HeadersSyncParams{
.commitment_period = 632,
...

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2323585831)
Does https://github.com/bitcoin/bitcoin/blob/eb0f41ee1ed9ea54019fcaa4e3ce33481da4459d/src/headerssync.h#L183 need updating?

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2323633980)
I'm still not comfortable with this assert here, can we assign it in the body or do an immediately-invoked lambda instead?
```suggestion
m_commit_offset{[&]() {
assert(params.commitment_period > 0);
return FastRandomContext().randrange(params.commitment_period);
}()},
```
Note that in this case the comment can also be eliminated

💬 davidgumberg commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3255946507)
I don't see how abusing the semantics of `AUTHOR_WARNING` is preferable to adding our own flag (#31665), but I prefer fixing #31476, so not a big deal.

---

I think the changes implemented by @stickies-v in https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings might make sense individually, but the sum total of this approach is that it shifts the experience of building bitcoin core from `cmake -B build` doin
...