👍 l0rinc approved a pull request: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3186721892)
Concept ACK

💬 l0rinc commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#discussion_r2323169139)
We might also want to mention that `doc/build-*.md` contains instructions for installing this new dependency, should they choose to not add `-DENABLE_IPC=OFF`

💬 davidgumberg commented on pull request "gui: Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3255145030)
> You implying that an upstream bug must be searched in Qt?

"Bug" might be too strong, having one HTML element be so large is probably pretty unsually behavior for users of `QTextEdit`, but I can't say for sure what is going wrong in [`QTextMarkdownWriter::writeFrame`](https://github.com/qt/qtbase/blob/b617d1176593963a2a9ed21dd5d9a63e84a09400/src/gui/text/qtextmarkdownwriter.cpp#L95).


> possible nit:
>
> ```
> PlainCopyQTextEdit
> ^
> ```

I don't really agree, I figure
...

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323179848)
We have `self.test_list.pop(0)` at the beginning: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_runner.py#L724

👍 l0rinc approved a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082#pullrequestreview-3186748868)
code review ACK d3c5e47391e2f158001e3e199d625852c7f18998

It also removes an objectionable translation 👍

👍 l0rinc approved a pull request: "doc: fix `LIBRARY_PATH` comment"
(https://github.com/bitcoin/bitcoin/pull/33308#pullrequestreview-3186756095)
code review ACK a2a35b58cb957b7fba113eb4cf933d5e7b9bf8ef

💬 sipa commented on pull request "doc: archive v29.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255200938)
@l0rinc This is just an unmodified copy from the release notes from the 29.x branch: https://github.com/bitcoin/bitcoin/blob/v29.1/doc/release-notes.md, so they survive in the master branch.

💬 laanwj commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3255228389)
> Is it worth trying every 5 minutes forever instead of giving up after a number of tries? Maybe just log unconditionally once but still try ever 5 minutes?

Yes, imo it is. And i don't think exponential backoff would be good here, either. If the user switches to a network with a PCP-supporting router, we want to use that as soon as possible.

But moving the noisy failure messages to `net` category makes sense, imo.

💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2323203417)
could you please separate the low risk changes from the riskier ones? This extraction seems like a simple refactor, would be great to see this in a very simple first commit, so that we can focus on the new call in `src/index/coinstatsindex.cpp`

💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2323209517)
as other have also mentioned, compaction is already a background process, it is triggered regularly, it should already take care of these as far as I can tell.
Is it urgent to do the expensive compaction regularly? Doing more compactions should take more time overall, can you please share your benchmarks in this regard?

💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2323218302)
is `const` the right thing to advertise here? And is it a `noexcept`?

💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2323214015)
shouldn't this be a debug? Do we need 91 instances of this?
(haven't checked, but will this be triggered for genesis?)

💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2323228522)
nit: to avoid counting the `0`s manually:
```C++
if (block.height % 10'000 == 0) {
```
or
```C++
if (!(block.height % 10'000)) {
```

💬 willcl-ark commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3255266380)
> > Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
>
> I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: [bitcoin/bitcoin/actions/runs/17471191651/workflow](https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow):
>
> ```
> .github/workflows/ci.yml
>
> Invalid workflow file
>
> (Line: 21, Col: 1): Unexpected value
...

💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323259358)
> We have `self.test_list.pop(0)` at the beginning:

This will already throw an IndexError. There is no need to check it manually twice for it. The check here is for `self.jobs`, to avoid an infinite `while True` loop in case of a coding error.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2323258878)
what happened here?

👍 willcl-ark approved a pull request: "doc: fix `LIBRARY_PATH` comment"
(https://github.com/bitcoin/bitcoin/pull/33308#pullrequestreview-3186851296)
ACK a2a35b58cb957b7fba113eb4cf933d5e7b9bf8ef

📝 luke-jr opened a pull request: "trace: Workaround GCC bug compiling with old systemtap"
(https://github.com/bitcoin/bitcoin/pull/33310)

💬 l0rinc commented on pull request "doc: archive v29.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255286967)
Ah, so that's what the `archive` was referring to, thanks.
Could we add that to the PR description?

📝 laanwj opened a pull request: "net: Quiet down logging when router doesn't support natpmp/pcp"
(https://github.com/bitcoin/bitcoin/pull/33311)
When the router doesn't support natpmp and PCP, one'd normally expect the UDP packet to be ignored, and hit a time out. This logs a warning that is already in the debug category. However, there's also the case in which sending an UDP packet causes a ICMP response. This is returned to user space as "connection refused" (despite UDP having no concept of connections).

Move the warnings from `Send` and `Recv` to debug level too, to reduce log spam in that case.

Closes #33301.