💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580997920)
nit: unnecessary extra newline

👍 stickies-v approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2024988118)
ACK e1be443315be6ba6c80267e0e6be59deee77de50

> The use-case for update is when you have a function that is performing multiple operations and you want to accumulate warnings and errors from all the operations and return them.

I agree, and from what I've seen so far, I would prefer that to be the only use-case.

> I also looked at remaining two update calls in this PR, and thought they both were not good uses, so I removed them as well.

Ah, nice one. Given that chaining results is not
...

💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581037929)
> Not sure about changing the code for a broken compiler warning.

It's also slightly more readable because it checks `window_tx_count` only once.

> I am sure this warning already exists in other parts of the code

This is the only warning thrown.

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581040393)
> This is the only warning thrown.

What about all the ones I get, or others? For example: https://api.cirrus-ci.com/v1/task/5983141741985792/logs/make.log

💬 kevkevinpal commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2079403879)
ACK [e912717](https://github.com/bitcoin/bitcoin/pull/29948/commits/e912717ff63f111d8f1cd7ed1fcf054e28f36409)

looks good to me

⚠️ sdaftuar opened an issue: "qa: Support git worktrees when running the linters locally via Docker"
(https://github.com/bitcoin/bitcoin/issues/29972)
### Please describe the feature you'd like to see added.

I discovered that running this command (from `test/lint/README.md`):
```DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter```
doesn't work when trying to run it out of a git worktree:
```
fatal: not a git repository: /home/sdaftuar/bitcoin/.git/worktrees/2024-04-cluster-mempool-txgraph-rebased
```
(Though it did work for me when I tried checking
...

📝 alfonsoromanz opened a pull request: "[Test] Assumeutxo: ensure failure when importing a snapshot twice"
(https://github.com/bitcoin/bitcoin/pull/29973)
I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate.

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581051238)
Or https://github.com/bitcoin/bitcoin/issues/29359

💬 laanwj commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581051463)
It's important to be careful about behavior changes but i do think this is an improvement, using `requests.head` is better practice than calling curl then looking for a string in the output.

💬 brunoerg commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2079416479)
Rebased to re-run CI.

💬 brunoerg commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2079418264)
If this is just a refactor, why the `fix:` on commit message?

💬 laanwj commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2079426084)
This is a script used for the tests, so adding `test` tag.

💬 maflcko commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581039091)
nit: Is this line needed?

💬 fjahr commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079432927)
```
test/functional/feature_assumeutxo.py:401:1: W293 blank line contains whitespace
^---- failure generated from lint-python.py
```

💬 fjahr commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079433198)
Concept ACK

💬 theuni commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2079433585)
Neat!

💬 maflcko commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1581066427)
> Or maybe, even check it and raise an error. Eg.

Sure, happy to close this pull if someone creates an alternative, but I'll probably leave this pull as-is

💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581073867)
This should be shadowing the previous `ancestors` variable (why the compiler isn't complaining about this?). The else path can also access the variable when it is declared inside the if statement.

💬 alfonsoromanz commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079445888)
Thanks @fjahr. I just fixed the linting issue.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079450470)
I currently don’t sort or actually the ordering gets messed up due to parallel processing. I will probably include the possibility to sort the tweaks in the array.

The cut-through tweak index stores the txid for every tweak, so sorting them might not be that necessary. The cut through index also includes metadata like a dust indicator.