💬 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
...

📝 hebasto opened a pull request: "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`"
(https://github.com/bitcoin/bitcoin/pull/33312)
The warnings are false positive and have been fixed upstream. See: https://github.com/capnproto/capnproto/pull/2334.

This PR:

1. Disables the `UndefinedBinaryOperatorResult` clang-tidy check for source files generated by the `mpgen` tool.

2. Is an alternative to the draft https://github.com/bitcoin/bitcoin/pull/33281.

3. Fixes https://github.com/bitcoin/bitcoin/issues/33256.

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

✅ achow101 closed an issue: "natpmp: quiet down unconditional logging"
(https://github.com/bitcoin/bitcoin/issues/33301)

🚀 achow101 merged a pull request: "net: Quiet down logging when router doesn't support natpmp/pcp"
(https://github.com/bitcoin/bitcoin/pull/33311)

🤔 mzumsande reviewed a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3187440917)
Concept ACK

💬 mzumsande commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2323675550)
> allows a peer to stall block download

I guess this would only be a serious problem in the case where all of our other peers don't support compact block (because otherwise parallel compact blocks would allow us to download the block).

In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to `GETDATA` explaining that we don't wipe the `partialBlock` there on purpose?