💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2464997684)
remark: Wondered whether it would be fair to mark as `Misbehaving()` or disconnect them, but there's a tiny chance they may have recently been an HB peer (as remarked in https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3456385354).
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2464997684)
remark: Wondered whether it would be fair to mark as `Misbehaving()` or disconnect them, but there's a tiny chance they may have recently been an HB peer (as remarked in https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3456385354).
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466672619)
This line is unnecessary, it can be replaced with something like:
```suggestion
self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
```
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466672619)
This line is unnecessary, it can be replaced with something like:
```suggestion
self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
```
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466779388)
Duplicated from other test in same file, could be made class-local instead?
(Last arg is always left at default in this new outer test function).
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466779388)
Duplicated from other test in same file, could be made class-local instead?
(Last arg is always left at default in this new outer test function).
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466749706)
nit:
```suggestion
# match the given parameters for the peer at idx of a given node
```
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466749706)
nit:
```suggestion
# match the given parameters for the peer at idx of a given node
```
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466826137)
Maybe use a comment to point out that since we only send the header and not the whole block, we are not marked as an HB peer?
And/or use `assert_highbandwidth_states(node, idx=-1, hb_to=False)` another time at the end of this block to verify/clarify.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466826137)
Maybe use a comment to point out that since we only send the header and not the whole block, we are not marked as an HB peer?
And/or use `assert_highbandwidth_states(node, idx=-1, hb_to=False)` another time at the end of this block to verify/clarify.
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2465045694)
👆
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2465045694)
👆
💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458122631)
re: https://github.com/bitcoin/bitcoin/issues/33575#issue-3496102450
> 2. Add a new `BlockTemplate::interruptWait()` to be able to interrupt `waitNext()` without destroying the template.
This is implemented in #33676 and could use some testing, but hopefully will help resolve this issue and #33554.
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458122631)
re: https://github.com/bitcoin/bitcoin/issues/33575#issue-3496102450
> 2. Add a new `BlockTemplate::interruptWait()` to be able to interrupt `waitNext()` without destroying the template.
This is implemented in #33676 and could use some testing, but hopefully will help resolve this issue and #33554.
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470784762)
That is a good idea, but not related to my suggestion here :). Yes, that would be a good followup.
I don't think we should have a method that would cause the node to crash if `Start` is called more than once. I disagree and think just calling `Stop` at the beginning of `Start` would be safer than the current implementation.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470784762)
That is a good idea, but not related to my suggestion here :). Yes, that would be a good followup.
I don't think we should have a method that would cause the node to crash if `Start` is called more than once. I disagree and think just calling `Stop` at the beginning of `Start` would be safer than the current implementation.
🤔 ismaelsadeeq reviewed a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3390482262)
Concept ACK, thanks for the seperation from the initial index PR.
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3390482262)
Concept ACK, thanks for the seperation from the initial index PR.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470815283)
> nit in [30e5b01](https://github.com/bitcoin/bitcoin/commit/30e5b015c973c1ac375e1eae6649238c6c2e2b28): This seems wrong, as there is no boost usage here?
It is used as in https://github.com/bitcoin/bitcoin/blob/a36b9320300d1fa1e5dbd1b4d493acf3578ac11a/src/policy/truc_policy.cpp#L218 the type of `mempool_ancestors.begin()` is `boost::multi_index::detail::hashed_index_iterator<...>`.
However, the `detail` namespace should be hidden with a Boost-specifc mapping file.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470815283)
> nit in [30e5b01](https://github.com/bitcoin/bitcoin/commit/30e5b015c973c1ac375e1eae6649238c6c2e2b28): This seems wrong, as there is no boost usage here?
It is used as in https://github.com/bitcoin/bitcoin/blob/a36b9320300d1fa1e5dbd1b4d493acf3578ac11a/src/policy/truc_policy.cpp#L218 the type of `mempool_ancestors.begin()` is `boost::multi_index::detail::hashed_index_iterator<...>`.
However, the `detail` namespace should be hidden with a Boost-specifc mapping file.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470824383)
hehe, it seems we sent a message at a similar time and missed yours.
re `Interrupt()` removal suggestion:
I find it harder to reason about the current two stages shutdown procedure if we have to call to the same `Stop()` method twice. At that point it would be simpler to just wait for all tasks to finish on the first call and remove the second one. But that seems suboptimal as there is no rush to wait for them at that point of the shutdown sequence.
Also, a benefit of keeping `Interrupt
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470824383)
hehe, it seems we sent a message at a similar time and missed yours.
re `Interrupt()` removal suggestion:
I find it harder to reason about the current two stages shutdown procedure if we have to call to the same `Stop()` method twice. At that point it would be simpler to just wait for all tasks to finish on the first call and remove the second one. But that seems suboptimal as there is no rush to wait for them at that point of the shutdown sequence.
Also, a benefit of keeping `Interrupt
...
💬 plebhash commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458195100)
I'll give it a try soon
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3458195100)
I'll give it a try soon
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470835931)
> It is used as in
I understand, but I wonder what value there is in listing the detailed internal boost includes. The `mempool_ancestors` already requires the `txmempool.h` include, which contains all required boost includes. No other module will have to include them to be able to compile.
To me those just seems like bloat for no benefit and the downside of making it harder to change a mempool implementation detail, because headers all over the codebase will have to be adjusted to please
...
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470835931)
> It is used as in
I understand, but I wonder what value there is in listing the detailed internal boost includes. The `mempool_ancestors` already requires the `txmempool.h` include, which contains all required boost includes. No other module will have to include them to be able to compile.
To me those just seems like bloat for no benefit and the downside of making it harder to change a mempool implementation detail, because headers all over the codebase will have to be adjusted to please
...
👍 hodlinator approved a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3390537459)
re-ACK c62b40d7abd8de1972bacb8c600bbab55a231b20
Changes since my first ACK (https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3168171370):
* Adjusted docs based on my latest feedback.
* Avoids re-converting tx hash in the RPC code.
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3390537459)
re-ACK c62b40d7abd8de1972bacb8c600bbab55a231b20
Changes since my first ACK (https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3168171370):
* Adjusted docs based on my latest feedback.
* Avoids re-converting tx hash in the RPC code.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470845344)
> I don't think we should have a method that would cause the node to crash if Start is called more than once. I disagree and think just calling Stop at the beginning of Start would be safer than the current implementation.
Wouldn't you agree that if your code calls `Start()` twice, you have a design issue? Thread pools are typically started once, maintained for the entire app’s lifecycle, and reused across modules. Mainly because creating and destroying threads isn’t cheap. I think it’s fair
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470845344)
> I don't think we should have a method that would cause the node to crash if Start is called more than once. I disagree and think just calling Stop at the beginning of Start would be safer than the current implementation.
Wouldn't you agree that if your code calls `Start()` twice, you have a design issue? Thread pools are typically started once, maintained for the entire app’s lifecycle, and reused across modules. Mainly because creating and destroying threads isn’t cheap. I think it’s fair
...
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470851529)
So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470851529)
So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470853688)
You're right.
Reworked.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470853688)
You're right.
Reworked.
💬 hebasto commented on pull request "random: scope the environ extern to macOS":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3458248055)
Testing on some other platforms: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3458248055)
Testing on some other platforms: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
💬 frodrik-froggo commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458279276)
ACK
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458279276)
ACK
👍 maflcko approved a pull request: "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`"
(https://github.com/bitcoin/bitcoin/pull/33725#pullrequestreview-3390598306)
review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2
...
(https://github.com/bitcoin/bitcoin/pull/33725#pullrequestreview-3390598306)
review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2
...