💬 pinheadmz commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1214557458)
Thanks, both comments addressed with rebase
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1214557458)
Thanks, both comments addressed with rebase
🚀 fanquake merged a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790)
(https://github.com/bitcoin/bitcoin/pull/27790)
💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214568318)
> How about making `startShutdown` return a boolean then that can be interpreted by calling code to either terminate the function or continue?
I think it's best if the notification interfaces just streamed notifications in one direction to the application, to update its UI, or trigger tasks like starting other services, saving state, freeing up disk space, or shutting down.
The notifications interface seems like an awkward way to send information the other direction from the application t
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214568318)
> How about making `startShutdown` return a boolean then that can be interpreted by calling code to either terminate the function or continue?
I think it's best if the notification interfaces just streamed notifications in one direction to the application, to update its UI, or trigger tasks like starting other services, saving state, freeing up disk space, or shutting down.
The notifications interface seems like an awkward way to send information the other direction from the application t
...
👍 pinheadmz approved a pull request: "Raise on invalid -debug and -loglevel config options"
(https://github.com/bitcoin/bitcoin/pull/27632#pullrequestreview-1457942685)
ACK 1a27bec1e9e40166d6ff7f0492115107289e7609
Reviewed code and ran tests locally. Confirmed expected behavior locally with regtest:
```
--> bcd -regtest -debug=poop
Error: Unsupported logging category -debug=poop
--> bcd -regtest -loglevel=poop
Error: Unsupported global logging level -loglevel=poop. Valid values: info, debug, trace.
```
Just had a question about workflow/testing below, not a deal breaker.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MES
...
(https://github.com/bitcoin/bitcoin/pull/27632#pullrequestreview-1457942685)
ACK 1a27bec1e9e40166d6ff7f0492115107289e7609
Reviewed code and ran tests locally. Confirmed expected behavior locally with regtest:
```
--> bcd -regtest -debug=poop
Error: Unsupported logging category -debug=poop
--> bcd -regtest -loglevel=poop
Error: Unsupported global logging level -loglevel=poop. Valid values: info, debug, trace.
```
Just had a question about workflow/testing below, not a deal breaker.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MES
...
💬 pinheadmz commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1214574564)
If you're touching unit tests anyway, why not add a test for invalid args here as well?
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1214574564)
If you're touching unit tests anyway, why not add a test for invalid args here as well?
💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1214589081)
interesting idea, so we could add `NetPermissionFlags::NoBanForce` or something. The danger will still be present for users of the permission, but existing users of `NoBan` wouldn't have to worry. And since `NoBan` is a default of `whitebind` it'll require more user attention anyway to use the more dangerous option.
In that case, the release note should still warn the user about using `NoBanForce` (or whatever we call it) but that warning can just be general, like, keep an eye on your netinfo
...
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1214589081)
interesting idea, so we could add `NetPermissionFlags::NoBanForce` or something. The danger will still be present for users of the permission, but existing users of `NoBan` wouldn't have to worry. And since `NoBan` is a default of `whitebind` it'll require more user attention anyway to use the more dangerous option.
In that case, the release note should still warn the user about using `NoBanForce` (or whatever we call it) but that warning can just be general, like, keep an eye on your netinfo
...
💬 theuni commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1214591743)
Fixed upstream: https://github.com/llvm/llvm-project/commit/5b77e752dcd073846b89559d6c0e1a7699e58615 for clang 17.
:D
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1214591743)
Fixed upstream: https://github.com/llvm/llvm-project/commit/5b77e752dcd073846b89559d6c0e1a7699e58615 for clang 17.
:D
💬 zkfrio commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1574030615)
> It's important to point out that this PR doesn't add functionality that couldn't be achieved manually before.
If a user wanted to split a coin and send it to themselves, they could do it with the existing Bitcoin Core (either via RPC/Console or the GUI). It's just cumbersome and error prone.
Good point and most of the people who tried this with coins involved in any incident regret doing it. Privacy in bitcoin is not as simple as doing some transactions with your own addresses automaticall
...
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1574030615)
> It's important to point out that this PR doesn't add functionality that couldn't be achieved manually before.
If a user wanted to split a coin and send it to themselves, they could do it with the existing Bitcoin Core (either via RPC/Console or the GUI). It's just cumbersome and error prone.
Good point and most of the people who tried this with coins involved in any incident regret doing it. Privacy in bitcoin is not as simple as doing some transactions with your own addresses automaticall
...
💬 amitiuttarwar commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1214617536)
yeah, not the best but I think the chances of repeatedly not hitting the `Assume` condition to be extremely low. alternatively, I could add a counter to prevent that worst case if it seems worth the additional complexity.
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1214617536)
yeah, not the best but I think the chances of repeatedly not hitting the `Assume` condition to be extremely low. alternatively, I could add a counter to prevent that worst case if it seems worth the additional complexity.
💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214627508)
@amitiuttarwar sort of. It's two things:
1. If enum values are not explicitly assigned (which is the case for the `Network` enum right now), there is no guarantee that `NET_MAX` is what you think it will be. This came out of a conversation with @vasild during review earlier this year. It's my understanding that `NET_MAX` _probably_ corresponds to the total number of enum values, but even today there are no guarantees on the value of `NET_MAX`. I'd like to do a little more research here and ge
...
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214627508)
@amitiuttarwar sort of. It's two things:
1. If enum values are not explicitly assigned (which is the case for the `Network` enum right now), there is no guarantee that `NET_MAX` is what you think it will be. This came out of a conversation with @vasild during review earlier this year. It's my understanding that `NET_MAX` _probably_ corresponds to the total number of enum values, but even today there are no guarantees on the value of `NET_MAX`. I'd like to do a little more research here and ge
...
💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214629720)
Good point. Do you think it's okay to do
```
if (index >= NUM_NET_MESSAGE_TYPES) {
return NET_MESSAGE_TYPE_OTHER;
}
```
Or should I add a separate case for `if (index > NUM_NET_MESSAGE_TYPES)` that throws an error? Not in love with this because it's harder to get test coverage, but it certainly feels like the more correct thing to do.
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214629720)
Good point. Do you think it's okay to do
```
if (index >= NUM_NET_MESSAGE_TYPES) {
return NET_MESSAGE_TYPE_OTHER;
}
```
Or should I add a separate case for `if (index > NUM_NET_MESSAGE_TYPES)` that throws an error? Not in love with this because it's harder to get test coverage, but it certainly feels like the more correct thing to do.
💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214631468)
Sounds good! Will work on this now (leaving this reply in case this comment disappears once I rebase)
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214631468)
Sounds good! Will work on this now (leaving this reply in case this comment disappears once I rebase)
📝 SamuelMarks opened a pull request: "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration"
(https://github.com/bitcoin/bitcoin/pull/27805)
Note that `size_t` is not always `size_type`, but I haven't replaced your other usages of `size_t` throughout the codebase (I can upon request).
Some details:
- https://en.cppreference.com/w/cpp/container/vector#Member_types
- https://www.ibm.com/docs/en/zos/2.4.0?topic=types-vectorsize-type
Further discussion:
- https://stackoverflow.com/q/17258067
- https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1428r0.pdf a dissent by Bjarne Stroustrup
(https://github.com/bitcoin/bitcoin/pull/27805)
Note that `size_t` is not always `size_type`, but I haven't replaced your other usages of `size_t` throughout the codebase (I can upon request).
Some details:
- https://en.cppreference.com/w/cpp/container/vector#Member_types
- https://www.ibm.com/docs/en/zos/2.4.0?topic=types-vectorsize-type
Further discussion:
- https://stackoverflow.com/q/17258067
- https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1428r0.pdf a dissent by Bjarne Stroustrup
💬 MarcoFalke commented on pull request "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration":
(https://github.com/bitcoin/bitcoin/pull/27805#issuecomment-1574097015)
We don't support WIN16 or DOS, etc, so your rationale from the stackoverflow link does not apply. The paper you linked to talks about something else. I haven't looked at the other links.
Given that this doesn't change behavior or likely even the resulting binary, this qualifies as a style change:
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the
...
(https://github.com/bitcoin/bitcoin/pull/27805#issuecomment-1574097015)
We don't support WIN16 or DOS, etc, so your rationale from the stackoverflow link does not apply. The paper you linked to talks about something else. I haven't looked at the other links.
Given that this doesn't change behavior or likely even the resulting binary, this qualifies as a style change:
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the
...
✅ MarcoFalke closed a pull request: "[src/{core_read,core_write,psbt,txorphanage}.cpp] Use correct `size_type` for `std::vector` iteration"
(https://github.com/bitcoin/bitcoin/pull/27805)
(https://github.com/bitcoin/bitcoin/pull/27805)
💬 theuni commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#issuecomment-1574102337)
@fanquake and @hebasto have both pointed out to me in discussions that this works even without the `-nostdlibinc`. That is _purely_ accidental and coincidental, as the addition of `-nostdlibinc` is what's _intended_ to make the other changes here work.
By sheer luck though, they manage to put everything in the correct order such that the poisoned paths come last in the search-order. That feels brittle, but maybe good enough.
I'd also like to see what the consequences of the `argument unuse
...
(https://github.com/bitcoin/bitcoin/pull/27798#issuecomment-1574102337)
@fanquake and @hebasto have both pointed out to me in discussions that this works even without the `-nostdlibinc`. That is _purely_ accidental and coincidental, as the addition of `-nostdlibinc` is what's _intended_ to make the other changes here work.
By sheer luck though, they manage to put everything in the correct order such that the poisoned paths come last in the search-order. That feels brittle, but maybe good enough.
I'd also like to see what the consequences of the `argument unuse
...
💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214670321)
oof. some of the spacing is not quite right here even for cascading. I'm going to fix it in the new compilation unit/file, maybe then it will make a little more sense? I'm worried lining them up isn't going to be much help. It's hard to read too because the declaration for this 4D array happens by nesting arrays in arrays :sob:
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1214670321)
oof. some of the spacing is not quite right here even for cascading. I'm going to fix it in the new compilation unit/file, maybe then it will make a little more sense? I'm worried lining them up isn't going to be much help. It's hard to read too because the declaration for this 4D array happens by nesting arrays in arrays :sob:
📝 Xekyo opened a pull request: "fuzz: Fix mini_miner_selection running out of coin"
(https://github.com/bitcoin/bitcoin/pull/27806)
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior.
Fixed per belt-suspender approach:
- assert that available_coins is not empty before generating tx
- generate at least two coins per
...
(https://github.com/bitcoin/bitcoin/pull/27806)
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior.
Fixed per belt-suspender approach:
- assert that available_coins is not empty before generating tx
- generate at least two coins per
...
💬 sr-gi commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1574166059)
> > What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).
>
> This isn't trading off privacy -- that's the point of [this comment](https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1549711784). It does mean that so
...
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1574166059)
> > What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).
>
> This isn't trading off privacy -- that's the point of [this comment](https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1549711784). It does mean that so
...
💬 achow101 commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574167988)
Turning this on will log private keys as they are written to the db, which seems a little scary.
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574167988)
Turning this on will log private keys as they are written to the db, which seems a little scary.