💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581021830)
> Which seems spurious, but it goes away with e.g.:

Not sure about changing the code for a broken compiler warning. I am sure this warning already exists in other parts of the code, so if changing the code is a desire, then it should be done for the other places as well (and CI could check against it). However, my recommendation would be to report this to upstream with a minimal reproducer, if it still happens with g++14.0.1-beta

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581023235)
> However as you say, this not a problem when subtracting `nChainTx` of block (1) from `nChainTx` of block (7).

Ok, not sure what to do here then. I'll probably resolve the thread for now.

💬 maflcko commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2079383903)
Also:

## Getting started to contribute to Bitcoin Core

### Setting up your development environment

New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.

If you need
...

💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2079388456)
Posting my response to part of [your comment on #29906](https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078452007) here, since `Update()` is no longer relevant to that PR:

> Examples would be the [BlockManager::SaveBlockToDisk](https://github.com/ryanofsky/bitcoin/blob/4d2c9de24916f8d69514ea7c7251136e2762fa5c/src/node/blockstorage.cpp#L1167-L1195) function from https://github.com/bitcoin/bitcoin/pull/29700

Had just a quick look, but this looks like how I'd expect `Update()` to
...

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