💬 zPapaBear commented on issue "Error when launching Bitcoin Core":
(https://github.com/bitcoin/bitcoin/issues/29995#issuecomment-2083008473)
> Is this an external HDD? What is the filesystem?

Windows 10, internal HDD.

💬 maflcko commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083011036)
If you want to run locally, my recommendation would be to use the `test/lint/test_runner`.

💬 hebasto commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083018272)
> > How is it supposed to automatically catch cases when code changes make `#include <config/bitcoin-config.h>` unneeded in a header or source file?
>
> Yes, it is supposed to catch those. This is already the case and unrelated to this pull request. You can try it yourself. The output will be:
>
> ```
> ^^^
> None of the files use a symbol declared in the bitcoin-config.h header. However, they are including
> the header. Consider removing the unused include.
>
> ^---- ⚠️
...

💬 maflcko commented on issue "Error when launching Bitcoin Core":
(https://github.com/bitcoin/bitcoin/issues/29995#issuecomment-2083019057)
Which filesystem is the HDD that holds the datadir? For example, NTFS is a possible filesystem.

In any case, this error should not happen, unless there is a serious bug, or the data on the storage is corrupt. If `-reindex-chainstate` does not work, you can try `-reindex`, but they will only fight the symptom temporarily. The issue will come back at some point, because if it was a bug, it will remain unfixed, or if the storage is corrupt, it will also not be fixed by setting a flag in the sof
...

💬 maflcko commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083023858)
> The linter silently removes the added `#include <config/bitcoin-config.h> // IWYU pragma: keep` lines with no message.

Again, this is not about the linter. This is an issue with the docker setup, which I didn't write, and which is not changed in this pull request. If the issue persists in the lint/test_runner, then let me know. Otherwise, this is a separate issue.



> To elaborate my concerns, adding `// IWYU pragma: keep` precludes the IWYU from notifying us about the cases when the `
...

💬 zPapaBear commented on issue "Error when launching Bitcoin Core":
(https://github.com/bitcoin/bitcoin/issues/29995#issuecomment-2083024584)
> Which filesystem is the HDD that holds the datadir? For example, NTFS is a possible filesystem.
>
> In any case, this error should not happen, unless there is a serious bug, or the data on the storage is corrupt. If `-reindex-chainstate` does not work, you can try `-reindex`, but they will only fight the symptom temporarily. The issue will come back at some point, because if it was a bug, it will remain unfixed, or if the storage is corrupt, it will also not be fixed by setting a flag in th
...

💬 alfonsoromanz commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#discussion_r1583270129)
Maybe the line that should be removed here is this one? `- TODO: A descendant of the snapshot block`. I believe the scenario where the snapshot block is not on the most-work chain is addressed here: https://github.com/bitcoin/bitcoin/pull/29996

👍 hebasto approved a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535)
ACK fa4673c7dd465f18b1d7b2042262123992e846df, I have reviewed the code and it looks OK. Tested on Ubuntu 23.10.

📝 maflcko opened a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997)
The index-based Arg accessor is redundant with the name-based one. It does not provide any benefit to the code reader, or otherwise, so remove it.

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#discussion_r1583297727)
Done in https://github.com/bitcoin/bitcoin/pull/29997

💬 maflcko commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#discussion_r1583306274)
nit in 88b824a80b5c955285ef2ddc490e086bb40a890c: Given that you are adding features to the lint runner which are not trivially available in the docker way (above), it could make sense to move the docker option down by a section (only as a fallback), as it seems to have bugs anyway, according to https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2083018272

💬 stickies-v commented on pull request "doc: removed help text saying that peers may not connect automatically":
(https://github.com/bitcoin/bitcoin/pull/29994#discussion_r1583309265)
I don't think we should remove default ports or the I2P doc
```suggestion
argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
```

🤔 stickies-v reviewed a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2028885329)
Concept ACK. According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher.

📝 instagibbs opened a pull request: "functional test: ensure confirmed utxo being sourced for 2nd chain"
(https://github.com/bitcoin/bitcoin/pull/29998)
The test could fail/stop testing what we want if non-confirmed utxos become sourced through some internal change to `MiniWallet`; better to just fetch confirmed explicitly.

💬 itornaza commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2083124545)
tested re-ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d

This PR not only adds meaningful functionality by enabling seeds that are provided by the user to take precedence over the default DNS seeds, but also enhances user experience with relevant reporting on what exactly happens through the process of connecting to the P2P network and which type of nodes are effectively used.

Built this PR from a cleaned up environment using `make distclean` and checked out the latest commit to test upon. M
...

💬 sdaftuar commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583360322)
I think that works, let me know if what I pushed matches what you're thinking!

👍 instagibbs approved a pull request: "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py"
(https://github.com/bitcoin/bitcoin/pull/29986#pullrequestreview-2028979977)
ACK f8a141c2dae2471a7ce7248e28a0bbeb8a291acd

👍 instagibbs approved a pull request: "fuzz: txorphan tests fixups"
(https://github.com/bitcoin/bitcoin/pull/29974#pullrequestreview-2028986127)
ACK 6998f02c97c0b2c3579fac160192402c8b09613d

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1583373054)
> Happy to have a go after this gets merged.

Implemented in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2024-04/move-warnings-node

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1583375437)
done