💬 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.
(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?
(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
(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)
(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?
(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.
(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.
💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323269567)
yes, but that's quadratic and `self.jobs` is not a list anymore, so the error is wrong - isn't it?
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323269567)
yes, but that's quadratic and `self.jobs` is not a list anymore, so the error is wrong - isn't it?
👍 darosior approved a pull request: "net: Quiet down logging when router doesn't support natpmp/pcp"
(https://github.com/bitcoin/bitcoin/pull/33311#pullrequestreview-3186874451)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
(https://github.com/bitcoin/bitcoin/pull/33311#pullrequestreview-3186874451)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
💬 willcl-ark commented on pull request "[29.x] *san CI backports":
(https://github.com/bitcoin/bitcoin/pull/33294#issuecomment-3255304727)
Are you planning on adding these to release notes in this PR?
(https://github.com/bitcoin/bitcoin/pull/33294#issuecomment-3255304727)
Are you planning on adding these to release notes in this PR?
💬 maflcko commented on pull request "doc: archive v29.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255307255)
> Could we add that to the PR description?
It is documented in the process: https://github.com/bitcoin/bitcoin/blob/v29.1/doc/release-process.md#after-6-or-more-people-have-guix-built-and-their-results-match
(https://github.com/bitcoin/bitcoin/pull/33309#issuecomment-3255307255)
> Could we add that to the PR description?
It is documented in the process: https://github.com/bitcoin/bitcoin/blob/v29.1/doc/release-process.md#after-6-or-more-people-have-guix-built-and-their-results-match
💬 willcl-ark commented on pull request "net: Quiet down logging when router doesn't support natpmp/pcp":
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255334801)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
Makes sense to demote this message to Debug level.
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255334801)
utACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
Makes sense to demote this message to Debug level.
💬 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
...