💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612022021)
If it appears after the IPv4 gateway log message it's for the IPv4 gateway, if it's after the IPv6 one it's for the IPv6 gateway. No more than two gateways are ever used. i don't think adding it to every log message is worth it.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612022021)
If it appears after the IPv4 gateway log message it's for the IPv4 gateway, if it's after the IPv6 one it's for the IPv6 gateway. No more than two gateways are ever used. i don't think adding it to every log message is worth it.
💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2127624772)
> > did that. cleaner and atomic. thanks.
>
> Hmm, it's not that way in your new push tho
Ouch... pushed. thanks.
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2127624772)
> > did that. cleaner and atomic. thanks.
>
> Hmm, it's not that way in your new push tho
Ouch... pushed. thanks.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612025592)
i moved the cast to the if() inside the inner loop.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612025592)
i moved the cast to the if() inside the inner loop.
💬 luke-jr commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2127629483)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2127629483)
Concept ACK
💬 dergoegge commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1612041795)
I don't know how to make this work outside of these params. I don't want to use a macro, I think this should always be enabled if possible.
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1612041795)
I don't know how to make this work outside of these params. I don't want to use a macro, I think this should always be enabled if possible.
💬 maflcko commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612046682)
Looks like this is new code, but uncovered. It would be nice to have a test for this.
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612046682)
Looks like this is new code, but uncovered. It would be nice to have a test for this.
💬 maflcko commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612027063)
nit: It would be nice, if new code didn't use globals implicitly. What about storing a reference to the prams (or the magic) in a member field of this struct and using it here? An alternative would be to use params-serialization and embed the magic bytes during serialization.
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612027063)
nit: It would be nice, if new code didn't use globals implicitly. What about storing a reference to the prams (or the magic) in a member field of this struct and using it here? An alternative would be to use params-serialization and embed the magic bytes during serialization.
💬 maflcko commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612045699)
nit: I think this should check that the stream is completely empty, no? I suspect that a trailing byte will be left undetected, so it may be better to deserialize a `std::byte` here?
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1612045699)
nit: I think this should check that the stream is completely empty, no? I suspect that a trailing byte will be left undetected, so it may be better to deserialize a `std::byte` here?
🤔 ryanofsky reviewed a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2074413782)
Code review ACK 445e6de114c00311c506b9245adf7f63bbbbec1d. It was a little unclear in some places what behavior was supposed to be changing, and I think that could be documented better. But overall the changes look good and seem very positive.
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2074413782)
Code review ACK 445e6de114c00311c506b9245adf7f63bbbbec1d. It was a little unclear in some places what behavior was supposed to be changing, and I think that could be documented better. But overall the changes look good and seem very positive.
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612014056)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Not important, but since these arguments are being inserted in a map, it would probably be a little better to pass them by value instead of reference, and use std::move so they can be moved instead of copied.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612014056)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Not important, but since these arguments are being inserted in a map, it would probably be a little better to pass them by value instead of reference, and use std::move so they can be moved instead of copied.
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612033198)
In commit "node: update uiInterface whenever warnings updated" (396b261fc72d47d3ac4420d2db9910197a21d5ab)
Can commit message be updated to say what this change in behavior here is? It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad. It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it wo
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612033198)
In commit "node: update uiInterface whenever warnings updated" (396b261fc72d47d3ac4420d2db9910197a21d5ab)
Can commit message be updated to say what this change in behavior here is? It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad. It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it wo
...
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611957425)
In commit "move-only: move warnings from common to node" (6681395632c0ab5e048b34d63d26636856d4202d)
s/RPc/RPC/ in commit message
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611957425)
In commit "move-only: move warnings from common to node" (6681395632c0ab5e048b34d63d26636856d4202d)
s/RPc/RPC/ in commit message
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611964045)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Would be good to have release notes for this change mentioning new -alertnotify behavior, since it now can be trigged multiple times and in new cases
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611964045)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Would be good to have release notes for this change mentioning new -alertnotify behavior, since it now can be trigged multiple times and in new cases
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611973476)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
I guess this change also affects the order of warnings. It seems like previous code was written to consistently put the pre-release warning first, followed by the miscellaneous warnings, the large-work chain warning, and the time offset warning last.
Changing this is probably fine, but it would be good to note the change in the commit message. Could also note in setWarning d
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611973476)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
I guess this change also affects the order of warnings. It seems like previous code was written to consistently put the pre-release warning first, followed by the miscellaneous warnings, the large-work chain warning, and the time offset warning last.
Changing this is probably fine, but it would be good to note the change in the commit message. Could also note in setWarning d
...
💬 luke-jr commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612059228)
Is it a safe assumption that this cannot happen except an interrupted replay?
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612059228)
Is it a safe assumption that this cannot happen except an interrupted replay?
💬 luke-jr commented on pull request "Fee Estimation via Fee rate Forecasters":
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2127682430)
>Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information. Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2127682430)
>Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information. Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.
📝 sipa opened a pull request: "util: add RingBuffer"
(https://github.com/bitcoin/bitcoin/pull/30161)
Extracted from #30126.
This adds a `RingBuffer` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque).
It is intended for the candidate set search queue in #30126, but may be useful as a repla
...
(https://github.com/bitcoin/bitcoin/pull/30161)
Extracted from #30126.
This adds a `RingBuffer` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque).
It is intended for the candidate set search queue in #30126, but may be useful as a repla
...
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612065514)
not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.
For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn't crash the node, but notify the user over the RPC.
Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.
Also, I am not sure about the log messa
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612065514)
not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.
For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn't crash the node, but notify the user over the RPC.
Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.
Also, I am not sure about the log messa
...
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612068360)
Oh, now I see this is required, because otherwise the node would crash on this error instead of continuing with the current code, if this was missing?
Not sure this is desirable, see also https://github.com/bitcoin/bitcoin/pull/29307/files#r1612065514
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612068360)
Oh, now I see this is required, because otherwise the node would crash on this error instead of continuing with the current code, if this was missing?
Not sure this is desirable, see also https://github.com/bitcoin/bitcoin/pull/29307/files#r1612065514
💬 murchandamus commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612056701)
I thought sibling eviction was relevant in a TRUC Transaction constellation when there already exist an unconfirmed parent and unconfirmed child, and now a newly submitted transaction is a child of the same parent, but spends a different output than the original child, i.e. not actually in conflict with the original child except for the topological restrictions affecting TRUC Transactions.
If I got all that right, I would posit that "This only occurs in single transaction package settings." i
...
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612056701)
I thought sibling eviction was relevant in a TRUC Transaction constellation when there already exist an unconfirmed parent and unconfirmed child, and now a newly submitted transaction is a child of the same parent, but spends a different output than the original child, i.e. not actually in conflict with the original child except for the topological restrictions affecting TRUC Transactions.
If I got all that right, I would posit that "This only occurs in single transaction package settings." i
...