💬 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?

💬 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
...

💬 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?

💬 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
...

💬 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...

💬 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.

💬 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
...

💬 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
...

💬 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.

📝 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
...

💬 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
...

💬 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));
```

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(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.

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527764328)
Done

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527765721)
Added a more extensive test for this in a new commit.

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527766160)
Done

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527777820)
That's a bug, nice find! The line where I had `seek(pos, SEEK_END)` in the `size()` implementation should have been `seek(0, SEEK_END)`, this lead to these wrong numbers. In these tests the position was at the end (7) and `seek(pos, SEEK_END)` moved the pos to EOF plus `pos`, which was 14. Fixed!

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2527778252)
Done

💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3533149878)
Addressed all feedback from @hodlinator , thanks for the review!