💬 stringintech commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532613361)
By inconsistency, I mean that refactoring `set_options`/`set_level_category`/`enable_category`/etc. to receive a `btck_LoggingConnection*` parameter suggests that settings are supported per connection, while calling them on a newly instantiated logging connection would actually override settings for all previously instantiated connections. While the actual behavior is well-documented, the function signatures themselves may still create expectations of per-connection isolation.
I understand yo
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532613361)
By inconsistency, I mean that refactoring `set_options`/`set_level_category`/`enable_category`/etc. to receive a `btck_LoggingConnection*` parameter suggests that settings are supported per connection, while calling them on a newly instantiated logging connection would actually override settings for all previously instantiated connections. While the actual behavior is well-documented, the function signatures themselves may still create expectations of per-connection isolation.
I understand yo
...
💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527423684)
The BanMan methods used here take `m_banned_mutex` and never call back into `AddrMan`, so the lock order in this policy is currently `AddrMan::cs`>`BanMan::m_banned_mutex` with no reverse lock order in the codebase.
Using the predicate here also enforces that `AddrMan::cs` is always taken first, before any `BanMan` locks which helps maintain a consistent lock order.
`GetAddressesUnsafe()` is invoked on startup to load `peers.dat` with the responses being cached for `CConnman::GetAddresses(
...
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527423684)
The BanMan methods used here take `m_banned_mutex` and never call back into `AddrMan`, so the lock order in this policy is currently `AddrMan::cs`>`BanMan::m_banned_mutex` with no reverse lock order in the codebase.
Using the predicate here also enforces that `AddrMan::cs` is always taken first, before any `BanMan` locks which helps maintain a consistent lock order.
`GetAddressesUnsafe()` is invoked on startup to load `peers.dat` with the responses being cached for `CConnman::GetAddresses(
...
💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527436474)
Yes; Currently all skipped addresses in `AddrMan::GetAddr` are counted as `filtered`; If you'd prefer (or find it useful) separate counters for each reason (Network, Quality or Policy), I can add that.
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527436474)
Yes; Currently all skipped addresses in `AddrMan::GetAddr` are counted as `filtered`; If you'd prefer (or find it useful) separate counters for each reason (Network, Quality or Policy), I can add that.
🤔 rkrux reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3464805989)
Concept ACK 0cbcdcaf5a7fe1dd864d0019a7d7e4f8b80b5e7c
Makes sense to use the pre-mined chain and the MiniWallet for this use case.
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3464805989)
Concept ACK 0cbcdcaf5a7fe1dd864d0019a7d7e4f8b80b5e7c
Makes sense to use the pre-mined chain and the MiniWallet for this use case.
💬 rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527400928)
In d1a2e278572f70d0d68a21312b2cc7de27f326df "test: replace class ValidWitnessMalleatedTx with function"
Unlike the `malleate_tx_to_invalid_witness` function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn't seem generic enough to be present in an util file. Few points regarding the function I noticed:
1. The name doesn't reflect the implementation, the fu
...
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527400928)
In d1a2e278572f70d0d68a21312b2cc7de27f326df "test: replace class ValidWitnessMalleatedTx with function"
Unlike the `malleate_tx_to_invalid_witness` function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn't seem generic enough to be present in an util file. Few points regarding the function I noticed:
1. The name doesn't reflect the implementation, the fu
...
💬 rkrux commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527403577)
In 26577cac8b9cf53d5f69e4502346fd5881c92004 "test: use pre-generated chain"
Nit (not strong opinion): Can remove this line altogether as well because `False` is the default value.
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2527403577)
In 26577cac8b9cf53d5f69e4502346fd5881c92004 "test: use pre-generated chain"
Nit (not strong opinion): Can remove this line altogether as well because `False` is the default value.
💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527465258)
> A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic
This would occur not from a mistake made by the API consumer, but an error with the binding logic (in rust it would be an error with cc/bindgen) I think. That is to say, a rather improbable condition imo.
> In the earlier approach on which you commented (https://github.com/bitcoin/bitcoin/commit/0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527465258)
> A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic
This would occur not from a mistake made by the API consumer, but an error with the binding logic (in rust it would be an error with cc/bindgen) I think. That is to say, a rather improbable condition imo.
> In the earlier approach on which you commented (https://github.com/bitcoin/bitcoin/commit/0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527492985)
but aren't read and write symmetric now in that regard?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527492985)
but aren't read and write symmetric now in that regard?
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3532765861)
Minor nits applied
<details>
<summary>git diff ea998e18f6d64f485d4e2d85a1028474fb35120a 02df44465218a49cc488fd50bf05ab2fd203c69c
</summary>
```diff
@@ -114,7 +114,7 @@ BOOST_AUTO_TEST_CASE(skip_height_properties_test)
for(int i{1}; i < n; ++i) {
auto new_index = std::make_unique<CBlockIndex>();
new_index->pprev = block_index.back().get();
- BOOST_CHECK(new_index->pprev);
+ BOOST_REQUIRE(new_index->pprev);
new_index->nHeight = new_index
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3532765861)
Minor nits applied
<details>
<summary>git diff ea998e18f6d64f485d4e2d85a1028474fb35120a 02df44465218a49cc488fd50bf05ab2fd203c69c
</summary>
```diff
@@ -114,7 +114,7 @@ BOOST_AUTO_TEST_CASE(skip_height_properties_test)
for(int i{1}; i < n; ++i) {
auto new_index = std::make_unique<CBlockIndex>();
new_index->pprev = block_index.back().get();
- BOOST_CHECK(new_index->pprev);
+ BOOST_REQUIRE(new_index->pprev);
new_index->nHeight = new_index
...
💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527544369)
Mmh, not sure what you are trying to say with that. What is the symmetry there? I think I am missing something, can you try and elaborate a bit?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527544369)
Mmh, not sure what you are trying to say with that. What is the symmetry there? I think I am missing something, can you try and elaborate a bit?
💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527556716)
> This would occur not from a mistake made by the API consumer, but an error with the binding logic
Not all users will use language bindings, and regardless, I don't think the distinction between "direct usage" and "wrapped usage" is meaningful here. Our implementation has UB when a non-zero length is used with a nullptr, so our implementation is responsible for preventing the UB.
> a check for both null and empty could be done without checking length.
How? In the API, a string is defin
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527556716)
> This would occur not from a mistake made by the API consumer, but an error with the binding logic
Not all users will use language bindings, and regardless, I don't think the distinction between "direct usage" and "wrapped usage" is meaningful here. Our implementation has UB when a non-zero length is used with a nullptr, so our implementation is responsible for preventing the UB.
> a check for both null and empty could be done without checking length.
How? In the API, a string is defin
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527561867)
the comment states: "shutdown on LevelDB read errors [...] Writes do not need similar protection".
So now that reads and writes both throw and don't return, do we need to adjust the error catchers - since either reads also don't need protection anymore or writes also do - if I understand the catchers' purpose...
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527561867)
the comment states: "shutdown on LevelDB read errors [...] Writes do not need similar protection".
So now that reads and writes both throw and don't return, do we need to adjust the error catchers - since either reads also don't need protection anymore or writes also do - if I understand the catchers' purpose...
💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527590240)
I don't think we are changing behaviour here (though I might stand corrected still), so if a change is required to them, the error catchers would have been buggy from the beginning. I have long questioned though to how we catch the db write errors, and if it might lead to an infinite loop in some cases. The exception handling just doesn't seem very robust or thought-through to me.
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527590240)
I don't think we are changing behaviour here (though I might stand corrected still), so if a change is required to them, the error catchers would have been buggy from the beginning. I have long questioned though to how we catch the db write errors, and if it might lead to an infinite loop in some cases. The exception handling just doesn't seem very robust or thought-through to me.
💬 stickies-v commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532938522)
Concept ACK, approach ~0 leaning towards nack. I think the approach that merging the backwards-incompatible interface early is not unreasonable but not my preferred one, because:
- I don't think we should at all care about breaking interfaces at the moment, the kernel API is experimental and upstream development flexibility and pace should absolutely be prioritized until kernel is in a more stable state
- having the interface behave in unexpected ways is just not desirable, even if this gap is
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3532938522)
Concept ACK, approach ~0 leaning towards nack. I think the approach that merging the backwards-incompatible interface early is not unreasonable but not my preferred one, because:
- I don't think we should at all care about breaking interfaces at the moment, the kernel API is experimental and upstream development flexibility and pace should absolutely be prioritized until kernel is in a more stable state
- having the interface behave in unexpected ways is just not desirable, even if this gap is
...
💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533008173)
> This delay could mean logs won't be produced by context-free operations
Thanks, I'll look into this. I am a little surprised there would be any context-free operations except very basic ones that shouldn't log. It'd expect there to be some context object, even a minimal one, associated with all non-trivial operations to control things like logging, random number generation, caching, etc.
Of course I do understand your abstract concern that adding a connection parameter "suggests that set
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533008173)
> This delay could mean logs won't be produced by context-free operations
Thanks, I'll look into this. I am a little surprised there would be any context-free operations except very basic ones that shouldn't log. It'd expect there to be some context object, even a minimal one, associated with all non-trivial operations to control things like logging, random number generation, caching, etc.
Of course I do understand your abstract concern that adding a connection parameter "suggests that set
...
💬 vasild commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3533027320)
I can reproduce this. A cursory debug shows that `txin.scriptWitness` IsNull but `txin.scriptSig` is not empty:
https://github.com/bitcoin/bitcoin/blob/e221b2524659d22cc5a2b0d0023254115a7a5622/src/psbt.h#L1253-L1254
A transaction created via the RPC does not exhibit this problem. Only when created from the GUI.
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3533027320)
I can reproduce this. A cursory debug shows that `txin.scriptWitness` IsNull but `txin.scriptSig` is not empty:
https://github.com/bitcoin/bitcoin/blob/e221b2524659d22cc5a2b0d0023254115a7a5622/src/psbt.h#L1253-L1254
A transaction created via the RPC does not exhibit this problem. Only when created from the GUI.
📝 ryanofsky converted_to_draft a pull request: "kernel: Improve logging API"
(https://github.com/bitcoin/bitcoin/pull/33847)
Simplify kernel logging API and try to connect it to the rest of the kernel API by letting log options be set on `Logger` objects directly and `Logger` objects to be associated with `Context` objects directly. Also drop `logging_disable()` function and avoid having library accumulate log messages in a 1MB buffer by default.
Changes are intended to make the API a little less surprising and more future proof. Rationale is described in some more detail the commit message.
This change is not b
...
(https://github.com/bitcoin/bitcoin/pull/33847)
Simplify kernel logging API and try to connect it to the rest of the kernel API by letting log options be set on `Logger` objects directly and `Logger` objects to be associated with `Context` objects directly. Also drop `logging_disable()` function and avoid having library accumulate log messages in a 1MB buffer by default.
Changes are intended to make the API a little less surprising and more future proof. Rationale is described in some more detail the commit message.
This change is not b
...
💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533073827)
> prefer to convert this to draft
Sure happy to do that while there is conceptual discussion.
> having the interface behave in unexpected ways is just not desirable
I think I've pointed out practical ways the current interface behaves in unexpected ways and is counterintuitive, and practical ways this PR is an improvement. The concerns you and stringintech raised do seem plausible, but also vague so if there are any concrete scenarios you can think of where this PR might create a proble
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3533073827)
> prefer to convert this to draft
Sure happy to do that while there is conceptual discussion.
> having the interface behave in unexpected ways is just not desirable
I think I've pointed out practical ways the current interface behaves in unexpected ways and is counterintuitive, and practical ways this PR is an improvement. The concerns you and stringintech raised do seem plausible, but also vague so if there are any concrete scenarios you can think of where this PR might create a proble
...
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2527749355)
Ahh right, and we don't have to recheck `current_value` each time which will have 1 less atomic operation if there is a lost race. If you push again maybe touch up to:
```C++
size_t current_value{m_num_to_open};
size_t new_value;
do {
new_value = current_value > n ? current_value - n : 0;
} while (!m_num_to_open.compare_exchange_weak(current_value, new_value));
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2527749355)
Ahh right, and we don't have to recheck `current_value` each time which will have 1 less atomic operation if there is a lost race. If you push again maybe touch up to:
```C++
size_t current_value{m_num_to_open};
size_t new_value;
do {
new_value = current_value > n ? current_value - n : 0;
} while (!m_num_to_open.compare_exchange_weak(current_value, new_value));
```
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527752096)
Seems reasonable, done.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527752096)
Seems reasonable, done.
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527763616)
No, but upon looking into it I think this is still better. The `size()` function uses the current file handle while `file_size()` would create a new one which could create race conditions etc.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527763616)
No, but upon looking into it I think this is still better. The `size()` function uses the current file handle while `file_size()` would create a new one which could create race conditions etc.