💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323334963)
> quadratic
Ok, I see. Though, that seems unrelated? I am not touching `test_list` in this pull request (and also not in the line you commented on), so you should be able to create a separate pull (happy to review).
> so the error is wrong - isn't it?
Yes, the message is "wrong". Though, it always was "wrong" (intentionally). The only thing that matters is the `raise`, the message is mostly irrelevant.
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323334963)
> quadratic
Ok, I see. Though, that seems unrelated? I am not touching `test_list` in this pull request (and also not in the line you commented on), so you should be able to create a separate pull (happy to review).
> so the error is wrong - isn't it?
Yes, the message is "wrong". Though, it always was "wrong" (intentionally). The only thing that matters is the `raise`, the message is mostly irrelevant.
💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323365026)
> so you should be able to create a separate pull
yes, I think it should be done in a separate PR, should have clarified that.
> Yes, the message is "wrong".
I'm not blocking, I don't think it's important, but if you edit again, maybe we can remove/update the message to remove the confusion
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323365026)
> so you should be able to create a separate pull
yes, I think it should be done in a separate PR, should have clarified that.
> Yes, the message is "wrong".
I'm not blocking, I don't think it's important, but if you edit again, maybe we can remove/update the message to remove the confusion
💬 l0rinc commented on pull request "doc: archive v29.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255407984)
I am aware of the doc, just confused which step of the release process this was - it's why I suggested adding a description to the PR to help other reviewers. I don't see why that's a controversial take, I reviewed this because I want to help and documented the parts that I think could be helpful for other reviewers.
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255407984)
I am aware of the doc, just confused which step of the release process this was - it's why I suggested adding a description to the PR to help other reviewers. I don't see why that's a controversial take, I reviewed this because I want to help and documented the parts that I think could be helpful for other reviewers.
💬 hebasto commented on pull request "doc: fix `LIBRARY_PATH` comment":
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2323416795)
While the comment's amendment is correct, the comment itself should by applied to both `darwin*` and `mingw*` hosts.
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2323416795)
While the comment's amendment is correct, the comment itself should by applied to both `darwin*` and `mingw*` hosts.
🤔 instagibbs reviewed a pull request: "cluster mempool: control/optimize TxGraph memory usage"
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3186772025)
concept ACK, I refuse to do math
(https://github.com/bitcoin/bitcoin/pull/33157#pullrequestreview-3186772025)
concept ACK, I refuse to do math
💬 instagibbs commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2323216351)
```Suggestion
* removed), return the Cluster it is in and the level the Cluster has. Otherwise, return
```
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2323216351)
```Suggestion
* removed), return the Cluster it is in and the level the Cluster has. Otherwise, return
```
💬 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
(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
(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
...
(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
(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.
(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.
(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`.
(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
(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
...
(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?
(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.
(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
...
(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.
(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.
(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.