💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664112367)
I guess the script execution cache could be wrapped just like the signature cache, which has an internal mutex.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664112367)
I guess the script execution cache could be wrapped just like the signature cache, which has an internal mutex.
📝 ajtowns opened a pull request: "logging: Simplify edge cases in logging configuration"
(https://github.com/bitcoin/bitcoin/pull/30384)
Corrects the documentation for logging rpc and -debugexclude to match behaviour; removes the "0" and "" entries in `LOG_CATEGORIES_BY_STR`.
(https://github.com/bitcoin/bitcoin/pull/30384)
Corrects the documentation for logging rpc and -debugexclude to match behaviour; removes the "0" and "" entries in `LOG_CATEGORIES_BY_STR`.
💬 ajtowns commented on pull request "logging: Simplify edge cases in logging configuration":
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206050525)
See https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1718621969 for history of the documented-but-not-working behaviour.
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206050525)
See https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1718621969 for history of the documented-but-not-working behaviour.
💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206054620)
> But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
>
> It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
I don't think anything is broken here, or maybe we have dif
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206054620)
> But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
>
> It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
I don't think anything is broken here, or maybe we have dif
...
💬 ryanofsky commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206072773)
I wonder if there is a DraftBot parsing bug? It seems to be parsing https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2154910604 as a concept ack instead of a plain ACK, and it requested another review from me despite the ACK.
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206072773)
I wonder if there is a DraftBot parsing bug? It seems to be parsing https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2154910604 as a concept ack instead of a plain ACK, and it requested another review from me despite the ACK.
🤔 dergoegge reviewed a pull request: "Stratum v2 Template Provider (take 3)"
(https://github.com/bitcoin/bitcoin/pull/29432#pullrequestreview-2156406658)
Thank you for all the work you've put into this.
Approach NACK
I refuse to believe that re-implementing the entire networking stack, adding a noise protocol implementation and a new networking protocol is the best solution to having a non-RPC/non-poll based template provider.
After looking at this PR, I would propose the following interface additions to Bitcoin Core:
* We add a new zmq publisher, e.g. `-zmqpubtemplate`, which publishes block templates as soon as they become available
...
(https://github.com/bitcoin/bitcoin/pull/29432#pullrequestreview-2156406658)
Thank you for all the work you've put into this.
Approach NACK
I refuse to believe that re-implementing the entire networking stack, adding a noise protocol implementation and a new networking protocol is the best solution to having a non-RPC/non-poll based template provider.
After looking at this PR, I would propose the following interface additions to Bitcoin Core:
* We add a new zmq publisher, e.g. `-zmqpubtemplate`, which publishes block templates as soon as they become available
...
💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1664165837)
How is this not polling?
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1664165837)
How is this not polling?
💬 ryanofsky commented on pull request "logging: Simplify edge cases in logging configuration":
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206082134)
Will review, but #29798 is making a similar and maybe wider simplification, so you might want to check that out.
(https://github.com/bitcoin/bitcoin/pull/30384#issuecomment-2206082134)
Will review, but #29798 is making a similar and maybe wider simplification, so you might want to check that out.
💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2206100324)
@Sjors, I changed the test `sv2_connman_tests/client_tests` from https://github.com/bitcoin/bitcoin/pull/30332 to use mocked sockets instead of real ones from the operating system. See the top commit from here: https://github.com/vasild/bitcoin/commits/sv2_mocksock/, that compiles, but the test fails at some point. I will try to get it to pass, just sharing this early wip with you.
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2206100324)
@Sjors, I changed the test `sv2_connman_tests/client_tests` from https://github.com/bitcoin/bitcoin/pull/30332 to use mocked sockets instead of real ones from the operating system. See the top commit from here: https://github.com/vasild/bitcoin/commits/sv2_mocksock/, that compiles, but the test fails at some point. I will try to get it to pass, just sharing this early wip with you.
💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206100735)
> > But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
> > It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
>
> I don't think anything is broken here, or maybe we have
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2206100735)
> > But that approach breaks if you're initialising the logger based on a conf file that you manage with `ArgsManager` -- since `ArgsManager` needs to initialise its `cs_args`, which then needs a logger, but you're still trying to work out how to configure the logger at this point.
> > It also breaks for the various global mutexes that are still in the kernel (just `cs_main`, `g_best_block_mutex`? perhaps `DumpMutex()::dump_mutex`).
>
> I don't think anything is broken here, or maybe we have
...
📝 furszy opened a pull request: "p2p: send not_found msgs for unknown, pruned or unwilling to share blocks"
(https://github.com/bitcoin/bitcoin/pull/30385)
The 'not_found' message, introduced in protocol version 70001 (Bitcoin Core 0.8.0 - Feb 2013), serves as a response to a 'getdata' message when the receiving node does not have the requested data available for relay. Thus far, this has only been applied to transactions. This PR proposes extending its use to blocks as well.
Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node t
...
(https://github.com/bitcoin/bitcoin/pull/30385)
The 'not_found' message, introduced in protocol version 70001 (Bitcoin Core 0.8.0 - Feb 2013), serves as a response to a 'getdata' message when the receiving node does not have the requested data available for relay. Thus far, this has only been applied to transactions. This PR proposes extending its use to blocks as well.
Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node t
...
💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664231914)
`ai_flags` only had `AI_ADDRCONFIG` set so without it it would be 0 anyway right?
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664231914)
`ai_flags` only had `AI_ADDRCONFIG` set so without it it would be 0 anyway right?
💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664237280)
We still need to check again if we do get the error though so checking the error would be more performant (not always having to call `getaddrinfo` twice) but _more_ code.
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664237280)
We still need to check again if we do get the error though so checking the error would be more performant (not always having to call `getaddrinfo` twice) but _more_ code.
💬 maflcko commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206166257)
DrahtBot is open-source, so pull requests and bug reports are welcome. But I am not sure if the additional code is worth it for the rare case where someone write a comment containing `ACK` after they sent a proper review `ACK commit_hash`. If you care about the summary comment being correct, you can:
* Edit your comments after your review to not contain `ACK`, or
* Resubmit your review
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2206166257)
DrahtBot is open-source, so pull requests and bug reports are welcome. But I am not sure if the additional code is worth it for the rare case where someone write a comment containing `ACK` after they sent a proper review `ACK commit_hash`. If you care about the summary comment being correct, you can:
* Edit your comments after your review to not contain `ACK`, or
* Resubmit your review
💬 dergoegge commented on pull request "[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks":
(https://github.com/bitcoin/bitcoin/pull/30385#issuecomment-2206166538)
> Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node to wait (and stall if the request was for an IBD block) for 10 minutes until the request timeout is triggered.
Nodes advertise which blocks they have and what their current tip is, so the timeouts should only happen if the peer is misbehaving or if you disrespect the protocol by asking for things they don't have. Disconnec
...
(https://github.com/bitcoin/bitcoin/pull/30385#issuecomment-2206166538)
> Currently, when the remote peer does not have the requested block data, it ignores the 'getdata' inventory block request. This lack of response causes the requesting node to wait (and stall if the request was for an IBD block) for 10 minutes until the request timeout is triggered.
Nodes advertise which blocks they have and what their current tip is, so the timeouts should only happen if the peer is misbehaving or if you disrespect the protocol by asking for things they don't have. Disconnec
...
👍 maflcko approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2156620993)
re-ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce 🐈
<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: re-ACK ce8094246ee95232e9d8
...
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2156620993)
re-ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce 🐈
<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: re-ACK ce8094246ee95232e9d8
...
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664293289)
nit: (haven't tried), but in a follow-up this could use `m_rng`, because the lock is already taken IIRC.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664293289)
nit: (haven't tried), but in a follow-up this could use `m_rng`, because the lock is already taken IIRC.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664298064)
added
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664298064)
added
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664298204)
done, thanks
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664298204)
done, thanks
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664301048)
I suppose it's not super accurate to say that `UpdatedBlockTipSync` is the synchronous version of `UpdatedBlockTip`, as the point here is to fire whenever the chain tip changes at all, while `UpdatedBlockTip` skips some things. i.e. `InvalidateBlock` when our new tip is a step back instead of an advancement.
I've removed comments comparing it to `UpdatedBlockTip`, slightly changed the calling logic, and renamed it to `ActiveTipChange`.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1664301048)
I suppose it's not super accurate to say that `UpdatedBlockTipSync` is the synchronous version of `UpdatedBlockTip`, as the point here is to fire whenever the chain tip changes at all, while `UpdatedBlockTip` skips some things. i.e. `InvalidateBlock` when our new tip is a step back instead of an advancement.
I've removed comments comparing it to `UpdatedBlockTip`, slightly changed the calling logic, and renamed it to `ActiveTipChange`.