💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853092768)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

nit: snake_case

```suggestion
for (size_t vin_idx = 0; vin_idx < tx->vin.size(); ++vin_idx) {
```

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853101842)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

`spk_1` is unused.

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853099223)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

This comment doesn't make sense to me. It suggests that a different child transaction has spent the coin. But what actually happens in this case is that this transaction (which is already in the mempool) is the child of a transaction that is also in the mempool.

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853100524)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

This function appears to be unused.

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853099606)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

These should be scoped in the loop iterating the inputs.

🚀 achow101 merged a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338)

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

I think this makes sense for -blocksdir and could make sense for -walletdir. But if the datadir was initiially created by an older version of Bitcoin and since upgraded it won't actually have a wallets/ subdirectory and will just store wallets in the datadir directly. So it could be reasonable to support -nowalletdir or -walletdir="" as a ways to request the same behavior when opening an old datadir. However, current wal
...

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