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

🤔 ZKcash-IrishGov reviewed a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2830010473)
**[](https://github.com/solana-foundation/solana-improvement-documents/actions/workflows/ci.yml)[](https://github.com/solana-foundation/solana-improvement-documents/actions/workflows/ci.yml)**

📝 theuni opened a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467)
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.

This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally.
...

💬 Bue-von-hon commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2868232853)
@adamandrews1 @achow101
As per the comments, we'll hand over the task to a more suitable developer.
Instead, we plan to proceed with [issue-26756](https://github.com/bitcoin/bitcoin/pull/26756) (we're a developer group based in South-Korea).
This issue involves aligning the behavior of the AvailableCoins method with the getbalance method, which seems well-suited to our expertise.
If you believe this issue isn’t appropriate for us, we’d appreciate it if you could suggest alternative issues.
...

💬 hMsats commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2868397512)
@sipa these were only the last 30 blocks. In total I let bitcoin-29.0 run for 56 blocks but the performance is so bad compared to when I restart bitcoin-28.0 that something else must be going on. Maybe the debug.log gives you more information?