💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851914389)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850720954

I could be missing something but it seems pretty obvious to me that -nocolor should be the same as -color=never. I don't think this PR should change that, but it might make sense to have a PR in the future that removes unjustified uses of disallow_negation so options so parsing is more uniform and less surprising. It could be the same PR that adds disallow_negation to prevent -noblocksdir as suggested https://github.com/
...

👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2451143073)
Code review ACK 2bf7e59776fcfb2c8a477cf2abb59606f7b4aacc. This is a nice improvement over status quo, but I'm pretty sure two of the commits 21bc3a3657fea5ed44172963639ee8ca5cd8e46c and 879a8d3a6820a86e6faa8d91f92d0cf374700fb8 are more complicated than they need to be and less robust than they could be.

I posted a simplified [branch](https://github.com/ryanofsky/bitcoin/commits/review.31212.4-edit) ([diff](https://github.com/ryanofsky/bitcoin/compare/review.31212.4..review.31212.4-edit)) and
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853186542)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850778259

As long as you are only adding the test coverage *after* fixing the bug (like this PR) I think the question of whether the test changes are in the same commit or a separate one are basically a question of personal style, and different developers will have different preferences.

But a much better practice IMO is to add the new test coverage in a separate commit *before* fixing the bug. Then the bugfix commit will inclu
...

⚠️ Parkeragan opened an issue: " "$comment": "Newer unreleased libevent versions cause https://github.com/bitcoin/bitcoin/issues/30096","
(https://github.com/bitcoin-core/gui/issues/846)
https://github.com/Parkeragan/bitcoin/blob/master/vcpkg.json#L60

✅ achow101 closed an issue: "."
(https://github.com/bitcoin-core/gui/issues/846)

:lock: achow101 locked an issue: "."
(https://github.com/bitcoin-core/gui/issues/846)

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853320033)
Agree, I had understood the *essence* of this comment within the context it lies, now that I re-read the comment it seems confusing in the first glance and can be improved.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853349616)
Probably a remnant of the previous commits now that `ScriptToUniv` is used that essentially contains this functionality but this `ScriptToAddress` function still seems generic enough to be added but in a different commit (or PR because this PR doesn't use it). Though, I doubt adding a function without an usage would be accepted.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853366459)
Interesting edge case, agree it would be nice to show results for the blocks that can be found. Right now, passing in 1 available block and 1 pruned block in a pruned node results in an error.

```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getblockchaininfo
{
"chain": "main",
"blocks": 871458,
"headers": 871458,
"bestblockhash": "00000000000000000000f8ee686deb34ddfd6d888279a7f0b42f623f938745ea",
"difficulty": 102289407543323.8,
...
"pruned": true,

...

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853375325)
If this behaviour is incorporated, then a test testing with a present block and a pruned block would be nice as well.

And on similar lines, this test `test_invalid_blockhash` can also be updated wherein an invalid block hash and a valid one are passed but result is still returned for the valid one instead of the RPC throwing an error like it does atm. Below the first block is invalid, the second valid.

```
➜ bitcoin git:(2024-08-getdescriptoractivity) ✗ bitcoinclimain getdescriptoractivi
...

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853379963)
Given that currently the CI checks every commit, do we have a way to specifying to the CI that the test commit is meant to fail, i.e. demonstrates the need for the upcoming fix? Or would we add an ignored test before the fix and just unignore it with the fix? I still think that in the current setup it makes more sense to commit them as a singe work-unit - fixes and testing and benching and documenting them are not extras, they're part of the same work unit.

💬 maflcko commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493082250)
Fixed the docs in https://github.com/llvm/llvm-project/commit/994c544c18c86cbdb6536aae5d27ef7e2f592486

💬 Sjors commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2493170407)
re-utACK 73db95c65c1d372822166045ca8b9f173d5fd883

I also checked that `submitblock` in the new test fails with `duplicate` if you revert 1f7fc738255205a64374686aca9a4c53089360f1.

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1853497933)
Good catch, pushed...

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2493230777)
I re-enabled multiprocess for two more depends build machines: macOS cross and centos. Let's see how those go.

I also enabled it for the native fuzz with valgrind job and the native valgrind job, let's see how that goes.

For all non-depends builds I now explicitly set `-DWITH_MULTIPROCESS` to either `ON` or `OFF` for clarity.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853544511)
"being opened to the ".bitcoin"-directory (not a file)). " - double parenthesis close.

And if we're here aleady the other one is missing an `of` in `to track initialization *of* HTTP RPC authentication`

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853551532)
The part I'm not sure about is checking `gArgs.GetBoolArg("-version", false)` twice - since the inner check has an else, which is a bit confusing, since that can only happen if `argc < 2 || HelpRequested(gArgs)` are true - seems like the condition can be simplified, especially since `if (argc < 2) {` is rechecked later. (please verify for other such conditions, not necessarily this one)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853552851)
it's quite noisy in this case, but don't have string feelings about it :)

🤔 vasild reviewed a pull request: "test: avoid internet traffic in rpc_net.py"
(https://github.com/bitcoin/bitcoin/pull/31343#pullrequestreview-2453797824)
Concept ACK

💬 vasild commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1853553795)
I checked on my network and packets to `169.254.33.44` still leave the machine and are received by the default gateway.

What about using `-connect=0` or `-proxy=127.0.0.1:1` for this particular test? One of the nodes it adds is available and it can connect to it (node2), but by reading the test I get the impression that this is not necessary. I.e. the test would still work and fulfill its purpose if it cannot connect to node2. So a setup like `-proxy=127.0.0.1:1` where it cannot connect to an
...