💬 pinheadmz commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469965455)
c219b93c3b043de202bdf3c65b433fd17af2da89
belt-and-suspenders, could
`BOOST_CHECK_EQUAL(threadPool.WorkQueueSize(), 0);`
I just like the symmetry in a test since you assert the value is `20` before taking action.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469965455)
c219b93c3b043de202bdf3c65b433fd17af2da89
belt-and-suspenders, could
`BOOST_CHECK_EQUAL(threadPool.WorkQueueSize(), 0);`
I just like the symmetry in a test since you assert the value is `20` before taking action.
💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763)
> > 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.
Yeah, you're right. Though as glozow points out, it's also strange to have our sender/receiver side logic randomly mismatch depending on network behaviour (with this PR making the random mismatch much less frequent). Would it make sense to change the l
...
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763)
> > 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.
Yeah, you're right. Though as glozow points out, it's also strange to have our sender/receiver side logic randomly mismatch depending on network behaviour (with this PR making the random mismatch much less frequent). Would it make sense to change the l
...
💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462304084)
> If they weren't shuffled on receipt, I think the self-announcement would always be processed.
I think shuffling helps avoid an attacker being able to (a) prevent honest nodes' addresses from being relayed (by filling up the buffer, so that later honest announcements get dropped) and (b) (easily?) detect how many addresses are being rate limited. So I don't think removing the shuffle would be a good idea.
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462304084)
> If they weren't shuffled on receipt, I think the self-announcement would always be processed.
I think shuffling helps avoid an attacker being able to (a) prevent honest nodes' addresses from being relayed (by filling up the buffer, so that later honest announcements get dropped) and (b) (easily?) detect how many addresses are being rate limited. So I don't think removing the shuffle would be a good idea.
💬 gmaxwell commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462316950)
Luke-jr's seed returns knots 29.2 nodes, and yet he claims knots 29.2 is based on the same code it currently won't return. https://x.com/LukeDashjr/status/1977355254510256135 even though the issue is now known to luke-jr the behavior hasn't been correct.
I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions. The only exclusions I see discussed are e.g. old versions that didn't support node witness or have other known issues that make them un
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462316950)
Luke-jr's seed returns knots 29.2 nodes, and yet he claims knots 29.2 is based on the same code it currently won't return. https://x.com/LukeDashjr/status/1977355254510256135 even though the issue is now known to luke-jr the behavior hasn't been correct.
I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions. The only exclusions I see discussed are e.g. old versions that didn't support node witness or have other known issues that make them un
...
⚠️ maflcko opened an issue: "ci: Lint task caching does not work?"
(https://github.com/bitcoin/bitcoin/issues/33735)
_Originally posted by @maflcko in [#33640](https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3458556346)_
I presume the same issue exists for the lint task. Though, the caching there does not seem to work at all. So it could make sense to make the caching work there, and then also add a retry loop there.
Otherwise, there could be Ubuntu APT timeouts like https://github.com/bitcoin/bitcoin/actions/runs/18861389218/job/53820273763?pr=33247#step:4:1053
(https://github.com/bitcoin/bitcoin/issues/33735)
_Originally posted by @maflcko in [#33640](https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3458556346)_
I presume the same issue exists for the lint task. Though, the caching there does not seem to work at all. So it could make sense to make the caching work there, and then also add a retry loop there.
Otherwise, there could be Ubuntu APT timeouts like https://github.com/bitcoin/bitcoin/actions/runs/18861389218/job/53820273763?pr=33247#step:4:1053
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473904846)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> I was wondering if there's any way to assert that the unblocked tasks are all executing on one single remaining worker... would it be insane to use a non-atomic `int` here?
hmm, even if the non-atomic int works, that doesn't really guarantee that the increment was done in the same thread.
If we want to be 100% correct, we should store the ids of the threads that processed the tasks on a syn
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473904846)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> I was wondering if there's any way to assert that the unblocked tasks are all executing on one single remaining worker... would it be insane to use a non-atomic `int` here?
hmm, even if the non-atomic int works, that doesn't really guarantee that the increment was done in the same thread.
If we want to be 100% correct, we should store the ids of the threads that processed the tasks on a syn
...
💬 ajtowns commented on issue "Decouple datacarrier size and count limits (Draft PR)":
(https://github.com/bitcoin/bitcoin/issues/33595#issuecomment-3462376140)
I think this PR would be better titled as "Make policy acceptance of multiple datacarrier outputs a configurable option", with the idea being to either introduce a boolean ("-multipledatacarrier=1" by default, allowing multiple datacarrier outputs; "-multipledatacarrier=0" as an option, reverting to the 29.x and earlier default of allowing one output per transaction, and "-datacarrier=0" to not allow any such outputs) or a count ("-datacarriercount=10000" by default, being effectively unlimited,
...
(https://github.com/bitcoin/bitcoin/issues/33595#issuecomment-3462376140)
I think this PR would be better titled as "Make policy acceptance of multiple datacarrier outputs a configurable option", with the idea being to either introduce a boolean ("-multipledatacarrier=1" by default, allowing multiple datacarrier outputs; "-multipledatacarrier=0" as an option, reverting to the 29.x and earlier default of allowing one output per transaction, and "-datacarrier=0" to not allow any such outputs) or a count ("-datacarriercount=10000" by default, being effectively unlimited,
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473946974)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> [As mentioned before](https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228839630), not a blocker but i feel like sleeps like this are test-flakiness waiting to happen ...
This is one of those "wait and see if something happens" scenarios (if any task gets processed). We expect no activity here since all workers are busy.
I'm not sure there is another way of doing this, but if it f
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473946974)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> [As mentioned before](https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228839630), not a blocker but i feel like sleeps like this are test-flakiness waiting to happen ...
This is one of those "wait and see if something happens" scenarios (if any task gets processed). We expect no activity here since all workers are busy.
I'm not sure there is another way of doing this, but if it f
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473982880)
Sounds good. Done as suggested. Thanks
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473982880)
Sounds good. Done as suggested. Thanks
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473984516)
Sure. Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473984516)
Sure. Done as suggested.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3462430016)
Oh, awesome extensive test and review @pinheadmz! you rock!
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3462430016)
Oh, awesome extensive test and review @pinheadmz! you rock!
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474000255)
thread sanitizer would likely pick up on it though if you made it non-atomic.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2474000255)
thread sanitizer would likely pick up on it though if you made it non-atomic.
💬 glozow commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462497890)
> I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions.
It is helpful to know that the claim of simply not returning recent versions is false, thank you for looking into this.
> But someone who fights and throws accusations to keep their dnsseed in?
> Aggression about being included
> seed operators must have a good working relationship with the project and be supportive of its success.
I certainly agree that these are points to consider
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462497890)
> I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions.
It is helpful to know that the claim of simply not returning recent versions is false, thank you for looking into this.
> But someone who fights and throws accusations to keep their dnsseed in?
> Aggression about being included
> seed operators must have a good working relationship with the project and be supportive of its success.
I certainly agree that these are points to consider
...
💬 dergoegge commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3462509273)
> but it’s quite useful to be able to troubleshoot individual crashes/timeouts locally
This will still be possible, because you can build the `fuzz` binary without libFuzzer for reproduction purposes (this is what we do in the macOS fuzz CI job). Unless the bugs only manifest when running the fuzzer due to non-determinism.
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3462509273)
> but it’s quite useful to be able to troubleshoot individual crashes/timeouts locally
This will still be possible, because you can build the `fuzz` binary without libFuzzer for reproduction purposes (this is what we do in the macOS fuzz CI job). Unless the bugs only manifest when running the fuzzer due to non-determinism.
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2474103060)
Thanks, taken
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2474103060)
Thanks, taken
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3462543390)
> this pull request conflicts with the following ones:
> https://github.com/bitcoin/bitcoin/pull/33629 (Cluster mempool by sdaftuar)
> https://github.com/bitcoin/bitcoin/pull/33591 (Cluster mempool followups by sdaftuar)
I think if you want to continue these refactors, it might be better if you picked code that doesn't conflict with Cluster Mempool or Kernel work, or things we are trying to ship in `v31`. Could instead do directories that rarely change, like `qt`, or `zmq`?
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3462543390)
> this pull request conflicts with the following ones:
> https://github.com/bitcoin/bitcoin/pull/33629 (Cluster mempool by sdaftuar)
> https://github.com/bitcoin/bitcoin/pull/33591 (Cluster mempool followups by sdaftuar)
I think if you want to continue these refactors, it might be better if you picked code that doesn't conflict with Cluster Mempool or Kernel work, or things we are trying to ship in `v31`. Could instead do directories that rarely change, like `qt`, or `zmq`?
👍 ryanofsky approved a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3268268351)
Code review ACK 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319, though I didn't look closely at the fuzz test. Sorry for the delay in reviewing this, I've been meaning to get to it for some time. I think all the changes seem good and look correct, but I did suggest a number of possible updates below, mostly simplifications that should make the PR smaller.
I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is
...
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3268268351)
Code review ACK 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319, though I didn't look closely at the fuzz test. Sorry for the delay in reviewing this, I've been meaning to get to it for some time. I think all the changes seem good and look correct, but I did suggest a number of possible updates below, mostly simplifications that should make the PR smaller.
I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293)
In commit "node/miner: allow generation of oversized templates" (bd9bed462e251a5d3bd6e376f25060aeba9539d4)
This ALLOW_OVERSIZED_BLOCKS tag approach seems reasonable, but not ideal because:
- It doesn't only allow larger blocks. It also stops enforcing coinbase sigops limits and stops enforcing minimum clamp values.
- It duplicates code between `BlockAssembler::BlockAssembler` constructors.
I think adding a new `BlockAssembler::Options` `bool allow_oversized_blocks{false}` option instea
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293)
In commit "node/miner: allow generation of oversized templates" (bd9bed462e251a5d3bd6e376f25060aeba9539d4)
This ALLOW_OVERSIZED_BLOCKS tag approach seems reasonable, but not ideal because:
- It doesn't only allow larger blocks. It also stops enforcing coinbase sigops limits and stops enforcing minimum clamp values.
- It duplicates code between `BlockAssembler::BlockAssembler` constructors.
I think adding a new `BlockAssembler::Options` `bool allow_oversized_blocks{false}` option instea
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472932269)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
I think it would probably be better for this to use unique_ptr instead shared_ptr (like fee_estimator and other members), so lifetime of the BlockTemplateCache is more explicit and predictable.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472932269)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
I think it would probably be better for this to use unique_ptr instead shared_ptr (like fee_estimator and other members), so lifetime of the BlockTemplateCache is more explicit and predictable.
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473223014)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would seem nice to implement these comparisons in `operator==` helpers or other functions directly attached to BlockCreateOptions and `BlockAssembler::Options` structs. This way if new members are added to the structs, the comparison functions should not get out of date.
Otherwise it would be good to have comments in the two structs pointing out that this function may need to be updated if new fi
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473223014)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would seem nice to implement these comparisons in `operator==` helpers or other functions directly attached to BlockCreateOptions and `BlockAssembler::Options` structs. This way if new members are added to the structs, the comparison functions should not get out of date.
Otherwise it would be good to have comments in the two structs pointing out that this function may need to be updated if new fi
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473203366)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would be good to have a comment saying this is intentionally not comparing the `coinbase_output_script` field and why that is safe (assuming it is safe).
Also would be good to point out this is intentionally skipping `test_block_validity`.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473203366)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)
Would be good to have a comment saying this is intentionally not comparing the `coinbase_output_script` field and why that is safe (assuming it is safe).
Also would be good to point out this is intentionally skipping `test_block_validity`.