💬 sipa commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2867811571)
Posthumous Concept ACK

💬 1440000bytes commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2867822561)
> Concept NACK

This makes no difference to PR as explained by Ava in https://x.com/achow101/status/1919467263855300733?t=UoIfqeBgI-3TDEAevorOvg&s=19

⚠️ kev009 opened an issue: "WITH_DBUS shouldn't be limited to Linux"
(https://github.com/bitcoin/bitcoin/issues/32464)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

With the move to CMake, WITH_DBUS becomes a Linux only feature:
`cmake_dependent_option(WITH_DBUS "Enable DBus support." ON "CMAKE_SYSTEM_NAME STREQUAL \"Linux\" AND BUILD_GUI" OFF)`

### Expected behaviour

This should be enabled on any UNIX-like. Or alternatively disabled on WIN32.

### Steps to reproduce

cmake -DWITH_GUI -DWITH_DBUS on FreeBSD or other UNIX

### Relevant log output

_
...

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082486420)
> Or improve the code to not need so many comments in the first place?

So IIUC having the comment itself is a downside? I think different people use comments differently. I like when comments are there to help me navigate code and call out things that are notable, even repeat things that are "clear," because I find most comments easier to read than most code, and I like when I can get an overview of what is happening by just reading the comments and not even looking at the code. I think other
...

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082492235)
Yes, I usually treat comments as admitting defeat, since if we thought the code was obvious, we wouldn't feel the need to add more explanation to it.
But here it's already over-explained, what's the exact reason for wanting even more comments (but not even considering code changes like `GetSigOpCount(/*fAccurate*/false)` - which isn't just a dead comment since linters are checking it)?

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082510695)
> But here it's already over-explained, what's the exact reason for wanting even more comments

I think we probably disagree that the current code is overexplained. At least 3 of us here think that current code is confusing and a04f17a1882407db09b0a07338e12877ac1d9e92 is an improvement.

> (but not even considering code changes like `GetSigOpCount(/*fAccurate*/false)` - which isn't just a dead comment since linters are checking it)?

I didn't consider this just because I didn't know it was
...

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082514102)
It was suggested in https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081363131 already - but I can do it in a follow-up as well, since I'm working on optimizing these checks anyway - and if you think this comment helps, let's add it, I won't block.

🤔 hodlinator reviewed a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2828025318)
Thank you @ismaelsadeeq for your feedback & @ryanofsky for having mercy on this PR. :)

Aiming to address some of the remaining feedback in a follow-up PR next week.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2082520348)
Since it's been merged already, I'll add some more context here:

AFAIK all existing calls to `assert_raises_message` had `message==None`. In this PR I wanted to use it in a later commit. The old way of checking `e.error['message']` instead of `repr(e)` for occurrences of `message` was assuming a certain structure/layout of the exception type. Not sure if there's more to say.

I'm not married to using `repr(e)` (as indicated https://github.com/bitcoin/bitcoin/pull/30660#discussion_r206162010
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2082499849)
The ignored errors map are more of a categorization, while I intentionally wanted to include the full representation for the one latest error. I agree there is a drawback in them looking like different errors. Maybe something like this would be better?

```
AssertionError: [node 0] Unable to connect to bitcoind after 60s. Ignored errors: {'missing_credentials': 240}, latest error: 'missing_credentials'/ValueError('No RPC credentials').
```

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2082543230)
One remaining `\n`, which would probably fail on Windows. Agree it is marginally more readable.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2081515988)
I know right? :) Was aiming for speed, not pseudo-collision-resistance.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2082530009)
> I think I will prefer the previous one as there is a case where the test node has start without a cookie file

If you can pinpoint a specific test that calls `stop_node()` without setting up some kind of RPC connection, let me know. Without being able to send the stop-RPC, we have no way of signalling the node to shut down AFAIK. We should be calling something like `TestNode.kill_process()` instead for such cases. So, seems like a logic error worthy of an assert.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2081519646)
I like your final version for its simplicity and also being more correct as currently we only mutate the first found instance.

💬 walkjivefly commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2867949544)
Concept NACK.

There's already more than enough garbage in the Bitcoin chain thanks to the Ordinals Taproot hijacking. There's no need to make it even easier for people to spam the chain.

💬 achow101 commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2867954774)
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d

🚀 achow101 merged a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155)

💬 achow101 commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2868051681)
ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1

> Good point. It's used in at least the `TransactionView::bumpedFee` signal, which might be broken now.

Tested that manually and it seems to work as expected. There is no change in visual behavior between this PR and master when fee bumping from the gui.

📝 theuni opened a pull request: "thread-safety: fix annotations with REVERSE_LOCK"
(https://github.com/bitcoin/bitcoin/pull/32465)
This is one of several PRs to cleanup/modernize our threading primitives.

While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly.

Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference betw
...

📝 theuni opened a pull request: "threading: drop CSemaphore in favor of c++20 std::counting_semaphore"
(https://github.com/bitcoin/bitcoin/pull/32466)
This is relatively simple, but done in a bunch of commits to enable scripted diffs.

I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)

This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSe
...