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

👍 TheCharlatan approved a pull request: "guix: swap `moreutils` for just `sponge`"
(https://github.com/bitcoin/bitcoin/pull/31323#pullrequestreview-2454001885)
ACK c4ee9b8aa07824610e1de9c70a4b938844c5ff53

💬 hebasto commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#discussion_r1853651388)
Why are these lines needed?

👍 TheCharlatan approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2454014018)
ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f

Tested in the GUI too.

💬 fanquake commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493367729)
> which inadvertently broke coverage builds for both [Clang's source-based code coverage](https://issues.oss-fuzz.com/issues/379122777)

Can you clarify that this is only for oss-fuzz, due to its use of other options and a third party script, otherwise, source based code coverage using Clang currently still works fine with master (see: https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693). Similarly, in the inline comment you've added, can you be a bit more specific than "can
...