💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469577973)
in 83c8753abf9c2af75cfbacd5488605332da588a4, I think this is missing a few words?
```suggestion
/** How many linearization iterations required for TxGraph clusters to have "acceptable" quality, if they cannot be optimally linearized with fewer iterations. */
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469577973)
in 83c8753abf9c2af75cfbacd5488605332da588a4, I think this is missing a few words?
```suggestion
/** How many linearization iterations required for TxGraph clusters to have "acceptable" quality, if they cannot be optimally linearized with fewer iterations. */
```
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469628917)
2812471c2bfce6215d6d58c1defd39c7a050edbd: Maybe deserves a comment
```suggestion
// PrioritiseTransaction calls stack on previous ones. Set the new transaction fee to be current modified fee + feedelta.
m_txgraph->SetTransactionFee(*it, it->GetModifiedFee() + nFeeDelta);
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469628917)
2812471c2bfce6215d6d58c1defd39c7a050edbd: Maybe deserves a comment
```suggestion
// PrioritiseTransaction calls stack on previous ones. Set the new transaction fee to be current modified fee + feedelta.
m_txgraph->SetTransactionFee(*it, it->GetModifiedFee() + nFeeDelta);
```
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469865825)
in 0e25736d6e0
It would help the fuzzer run faster to just call `check` once at the end, instead of after each round of potential removals.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469865825)
in 0e25736d6e0
It would help the fuzzer run faster to just call `check` once at the end, instead of after each round of potential removals.
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469950583)
If not squashed, perhaps move it earlier so that the test can have the correct string when introduced?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469950583)
If not squashed, perhaps move it earlier so that the test can have the correct string when introduced?
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469612014)
nit 91d9bfcca62bd6c168476f6af442bd7049ff872e
I was going to complain about this magic number, maybe squash 83c8753abf9c2af75cfbacd5488605332da588a4 into its parent?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469612014)
nit 91d9bfcca62bd6c168476f6af442bd7049ff872e
I was going to complain about this magic number, maybe squash 83c8753abf9c2af75cfbacd5488605332da588a4 into its parent?
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469945254)
It seems a bit unfortunate that, prior to 675fa79bb280129fb87e5d1884e670a35a03f5cd, there isn't a cap on ancestor count when we call `CalculateMemPoolAncestors`. Could the rework be done earlier?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469945254)
It seems a bit unfortunate that, prior to 675fa79bb280129fb87e5d1884e670a35a03f5cd, there isn't a cap on ancestor count when we call `CalculateMemPoolAncestors`. Could the rework be done earlier?
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469604073)
Yes, calling `UpdateModifiedFee(0)` does nothing. I think it's fine to keep this :shrug:
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469604073)
Yes, calling `UpdateModifiedFee(0)` does nothing. I think it's fine to keep this :shrug:
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469981199)
> Isn't this comment already implied by `GUARDED_BY(m_mutex)`? Also it's written by the control thread and read by worker threads, so of course it must only be modified while holding the mutex. This comment doesn't add anything IMO.
What I meant there is that the variable shouldn’t be an atomic bool alone (in case someone propose to change it in the future). It must share the same mutex as the condition variable; otherwise, workers might not see the update. This is stated in the `std::conditi
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469981199)
> Isn't this comment already implied by `GUARDED_BY(m_mutex)`? Also it's written by the control thread and read by worker threads, so of course it must only be modified while holding the mutex. This comment doesn't add anything IMO.
What I meant there is that the variable shouldn’t be an atomic bool alone (in case someone propose to change it in the future). It must share the same mutex as the condition variable; otherwise, workers might not see the update. This is stated in the `std::conditi
...
💬 fanquake commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562)
Given the upstream fix has landed (and was possibly backported?), this workaround should either be removed entirely, or updated to indicate the version of Clang which this was fixed in.
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562)
Given the upstream fix has landed (and was possibly backported?), this workaround should either be removed entirely, or updated to indicate the version of Clang which this was fixed in.
💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457076576)
> On some occasions, a peer will send us addresses a few seconds after the connection is established.
What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
An advantage of starting with an (almost) empty bucket is that someone attempting an address spamming attack has to pay for that attack by maintaining a connection to us. Increasing the initial bucket for outbound/manual connections but not inbound connections migh
...
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457076576)
> On some occasions, a peer will send us addresses a few seconds after the connection is established.
What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
An advantage of starting with an (almost) empty bucket is that someone attempting an address spamming attack has to pay for that attack by maintaining a connection to us. Increasing the initial bucket for outbound/manual connections but not inbound connections migh
...
💬 ajtowns commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470028771)
Would be better to have `IsWitnessProgram()` populate a span rather than a vector if we're trying to minimise allocations, as far as I can see.
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470028771)
Would be better to have `IsWitnessProgram()` populate a span rather than a vector if we're trying to minimise allocations, as far as I can see.
💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2470035274)
This is turning `CMAKE_SKIP_BUILD_RPATH` into `CMAKE_SKIP_INSTALL_RPATH`?
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2470035274)
This is turning `CMAKE_SKIP_BUILD_RPATH` into `CMAKE_SKIP_INSTALL_RPATH`?
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470045034)
> I'm unclear what the purpose of this function is. It interrupts the workers, like `Stop`, but doesn't join them or wait for them to finish.
> If the idea is to clear the workqueue and park the threads without being able to add more tasks, it does not accomplish that since the threads will exit their loops as well and not be parked on the cv.
Actually, the goal is to keep the current shutdown semantics unchanged. The process already follows two stages: first, we stop accepting new requests
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470045034)
> I'm unclear what the purpose of this function is. It interrupts the workers, like `Stop`, but doesn't join them or wait for them to finish.
> If the idea is to clear the workqueue and park the threads without being able to add more tasks, it does not accomplish that since the threads will exit their loops as well and not be parked on the cv.
Actually, the goal is to keep the current shutdown semantics unchanged. The process already follows two stages: first, we stop accepting new requests
...
💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3457121575)
Guix Build (aarch64):
```bash
8e4d9f4db54d26c8720b4f4031f83105f81cab36ede446cf31e1e17a302cc062 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
ae43ea7ec85c4929843a2c1edfa1df3b68ad9f991c23630590aa18e6de36ebd9 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
b75993d29f48a6e820d638067648a6b9e4d76a23848c640523365b379ea64b01 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu.tar.gz
7a582f
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3457121575)
Guix Build (aarch64):
```bash
8e4d9f4db54d26c8720b4f4031f83105f81cab36ede446cf31e1e17a302cc062 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
ae43ea7ec85c4929843a2c1edfa1df3b68ad9f991c23630590aa18e6de36ebd9 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
b75993d29f48a6e820d638067648a6b9e4d76a23848c640523365b379ea64b01 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu.tar.gz
7a582f
...
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2470046855)
The comment has been updated.
> and was possibly backported?
No, it wasn't.
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2470046855)
The comment has been updated.
> and was possibly backported?
No, it wasn't.
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3457125965)
Rebased and addressed [feedback](https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562) from @fanquake.
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3457125965)
Rebased and addressed [feedback](https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562) from @fanquake.
💬 Raimo33 commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470058888)
here the vector is totally useless. it's required by the `IsWitnessProgram()` API but not actually needed. I've made more significant changes on this private branch, for another PR: https://github.com/Raimo33/bitcoinknots/commits/optimize-tx-policy-verification-2/
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470058888)
here the vector is totally useless. it's required by the `IsWitnessProgram()` API but not actually needed. I've made more significant changes on this private branch, for another PR: https://github.com/Raimo33/bitcoinknots/commits/optimize-tx-policy-verification-2/
🤔 glozow reviewed a pull request: "doc: mention key removal in rpc interface modification"
(https://github.com/bitcoin/bitcoin/pull/32867#pullrequestreview-3389486508)
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
(https://github.com/bitcoin/bitcoin/pull/32867#pullrequestreview-3389486508)
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
🚀 glozow merged a pull request: "doc: mention key removal in rpc interface modification"
(https://github.com/bitcoin/bitcoin/pull/32867)
(https://github.com/bitcoin/bitcoin/pull/32867)
💬 mzumsande commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457197373)
> What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
Ignoring the initial self-announcement of the peer if it get mixed up with other addrs would feel strange.
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457197373)
> What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
Ignoring the initial self-announcement of the peer if it get mixed up with other addrs would feel strange.