💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214538307)
'> Thank you for your comprehensive answer. I think it boils down to: A fatal error should not necessarily have to trigger an interrupt for all kernel operations.
Yes, this is definitely a goal. Libbitcoinkernel should try to be a library, not an application or application framework. It should have a convenient way of cancelling operations and not force everything to use one shutdown variable.
But both the current PR and my branch do already support this. The main difference between the PR
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214538307)
'> Thank you for your comprehensive answer. I think it boils down to: A fatal error should not necessarily have to trigger an interrupt for all kernel operations.
Yes, this is definitely a goal. Libbitcoinkernel should try to be a library, not an application or application framework. It should have a convenient way of cancelling operations and not force everything to use one shutdown variable.
But both the current PR and my branch do already support this. The main difference between the PR
...
💬 virtu commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1573950719)
@0xB10C and I had a look at this today. Here's what we found so far:
1. We believe the issue is caused by [one or more of the assertions](https://github.com/bitcoin/bitcoin/blob/b22408df162a224d94ac54e8443b57ef3fd2ca72/test/functional/interface_usdt_mempool.py#L306-L307) failing in the `handle_rejected_event` callback (although we are not sure why they fail).
2. The test framework's `assert_equal()` communicates failed assertions via exceptions. Unfortunately exceptions raised in bcc callback
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1573950719)
@0xB10C and I had a look at this today. Here's what we found so far:
1. We believe the issue is caused by [one or more of the assertions](https://github.com/bitcoin/bitcoin/blob/b22408df162a224d94ac54e8443b57ef3fd2ca72/test/functional/interface_usdt_mempool.py#L306-L307) failing in the `handle_rejected_event` callback (although we are not sure why they fail).
2. The test framework's `assert_equal()` communicates failed assertions via exceptions. Unfortunately exceptions raised in bcc callback
...
💬 TheCharlatan commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214545050)
> Also in both cases, it would be better if the kernel wasn't deciding itself whether to return early, but instead asked the application through std::function callbacks. I.e. there could be a std::function callbacks for "I just a new chain tip, should I keep processing or return early?" that would be more general than having hardcoded -stopatheight logic in the kernel.
How about making `startShutdown` return a boolean then that can be interpreted by calling code to either terminate the funct
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214545050)
> Also in both cases, it would be better if the kernel wasn't deciding itself whether to return early, but instead asked the application through std::function callbacks. I.e. there could be a std::function callbacks for "I just a new chain tip, should I keep processing or return early?" that would be more general than having hardcoded -stopatheight logic in the kernel.
How about making `startShutdown` return a boolean then that can be interpreted by calling code to either terminate the funct
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1573955807)
No I still need to figure out what the last message https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1558537251 means, and finish #27409 to fix the ["Unfortunately the behavior for bitcoin-qt is stupider than bitcoind..."](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470284856) issue
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1573955807)
No I still need to figure out what the last message https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1558537251 means, and finish #27409 to fix the ["Unfortunately the behavior for bitcoin-qt is stupider than bitcoind..."](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470284856) issue
💬 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: