✅ achow101 closed an issue: "Use AppData\Local instead of AppData\Roaming"
(https://github.com/bitcoin/bitcoin/issues/2391)
(https://github.com/bitcoin/bitcoin/issues/2391)
🚀 achow101 merged a pull request: "system: use %LOCALAPPDATA% as default datadir on windows"
(https://github.com/bitcoin/bitcoin/pull/27064)
(https://github.com/bitcoin/bitcoin/pull/27064)
💬 maflcko commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2127543290)
In theory the consumer could call the `help` RPC (or a computer readable help, once there is one, see https://github.com/bitcoin/bitcoin/issues/29912) and use that to double check that the views match. However, this may be more work for consumers to implement, as opposed to a simple version check.
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2127543290)
In theory the consumer could call the `help` RPC (or a computer readable help, once there is one, see https://github.com/bitcoin/bitcoin/issues/29912) and use that to double check that the views match. However, this may be more work for consumers to implement, as opposed to a simple version check.
Concept ACK
💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2127548244)
reACK 2fd34ba504957331f5a08614b6e1f8317260f04d via range-diff
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2127548244)
reACK 2fd34ba504957331f5a08614b6e1f8317260f04d via range-diff
💬 achow101 commented on pull request "rpc: Optimize serialization and enhance metadata of dumptxoutset output":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2127566569)
ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2127566569)
ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127569909)
> I don't see where I would get a chunk of time to do that right now....
Sure, no rush. I'll probably take some time to pin this down. (I don't have an AWS account, so I can't test it, but maybe someone else has).
Some other ideas to test in the meantime:
* Try another filesystem instead of xfs
* Try the master branch (not for production, just for testing whether the issue still happens there)
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127569909)
> I don't see where I would get a chunk of time to do that right now....
Sure, no rush. I'll probably take some time to pin this down. (I don't have an AWS account, so I can't test it, but maybe someone else has).
Some other ideas to test in the meantime:
* Try another filesystem instead of xfs
* Try the master branch (not for production, just for testing whether the issue still happens there)
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612003469)
Right, let's add `sizeof(rt_msghdr)` manually instead of doing the +1 trick.
> That makes sense. Whereas rt_msghdr is fixed length I guess, so you're able to use rt + 1 above (I'm still a bit terified that compilers don't care).
If we can hardcode the assumption that `sa_len` is the first byte (which it is) then we can avoid casting and dereferencing sockaddr, anywhere.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612003469)
Right, let's add `sizeof(rt_msghdr)` manually instead of doing the +1 trick.
> That makes sense. Whereas rt_msghdr is fixed length I guess, so you're able to use rt + 1 above (I'm still a bit terified that compilers don't care).
If we can hardcode the assumption that `sa_len` is the first byte (which it is) then we can avoid casting and dereferencing sockaddr, anywhere.
✅ 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.