✅ achow101 closed an issue: "Possible savings in `dumptxoutset` serialization format (~20%)"
(https://github.com/bitcoin/bitcoin/issues/25675)
(https://github.com/bitcoin/bitcoin/issues/25675)
🚀 achow101 merged a pull request: "rpc: Optimize serialization and enhance metadata of dumptxoutset output"
(https://github.com/bitcoin/bitcoin/pull/29612)
(https://github.com/bitcoin/bitcoin/pull/29612)
💬 brunoerg commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1612008655)
I agree. I will address it.
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1612008655)
I agree. I will address it.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612015499)
Currently i have the following, having changed the pointer arithmetic to byte offsets:
```c++
// Iterate over messages (each message is a routing table entry).
for (size_t msgptr = 0; msgptr < buf.size(); ) {
const struct rt_msghdr* rt = (const struct rt_msghdr*)(buf.data() + msgptr);
// Iterate over addresses within entry, get destination and gateway (if present).
// Pointer to address data within message, starts after header.
size_t saptr = msgptr
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612015499)
Currently i have the following, having changed the pointer arithmetic to byte offsets:
```c++
// Iterate over messages (each message is a routing table entry).
for (size_t msgptr = 0; msgptr < buf.size(); ) {
const struct rt_msghdr* rt = (const struct rt_msghdr*)(buf.data() + msgptr);
// Iterate over addresses within entry, get destination and gateway (if present).
// Pointer to address data within message, starts after header.
size_t saptr = msgptr
...
💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1612017998)
fixed, thanks
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1612017998)
fixed, thanks
💬 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?