💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082406101)
I'm a bit hesitant, because this part of the code has more docs than usual, e.g.
```C++
/**
* Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
* as 20 sigops. With pay-to-script-hash, that changed:
* CHECKMULTISIGs serialized in scriptSigs are
* counted more accurately, assuming they are of the form
* ... OP_N CHECKMULTISIG ...
*/
unsigned int GetSigOpCount(bool fAccurate) const;

/**
* Accurately count sigOps, including sigOps in
...

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082414176)
This seems like a meaningful it improvement to me because it clearly states that the check is inaccurate, but if there are downsides to merging this let me know!

💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2082422282)
> clearly states that the check is inaccurate

Isn't that already clear by calling `bool fAccurate` with `false`? In a method called `GetLegacySigOpCount` with the doc:
```C++
/**
* Count ECDSA signature operations the old-fashioned (pre-0.6) way
* @return number of sigops this transaction's outputs will produce when spent
* @see CTransaction::FetchInputs
*/
```
? Seems to me the warnings are already all over the place.

If we need more warnings, can we improve the existing o
...

💬 achow101 commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2867783717)
ACK 1ee698fde2e8652d8eb083749edb8029c003b8db

🚀 achow101 merged a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436)

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