💬 fanquake commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1432888644)
@jessebarton want to followup here?

💬 fanquake commented on pull request "test: previous releases: add v24.0.1":
(https://github.com/bitcoin/bitcoin/pull/26586#issuecomment-1432890478)
Not sure what to do here. @Sjors?

💬 vasild commented on pull request "I2P network optimizations":
(https://github.com/bitcoin/bitcoin/pull/26837#issuecomment-1432902206)
Like usual, after this is deemed to have had enough review and testing it can be merged. It is not tied to or dependent on I2P releases.

💬 scgbckbone commented on pull request "explicit `-d/--delete` argument for verifybinaries/verify.py":
(https://github.com/bitcoin/bitcoin/pull/26985#issuecomment-1433007940)
didn't know about that, will have a look - thanks

📝 fanquake converted_to_draft a pull request: "More verbose warning for multiple network argument error."
(https://github.com/bitcoin/bitcoin/pull/26028)
This PR has bitcoin print a more verbose message when a network argument collisions happens.
I'm new to using the bitcoin software and was reconfiguring an old node. I also have limited experience at coding, this is my first PR on an open-source project, so any feedback on how to improve this process is welcomed.

If a chain argument is in the bitcoin.conf and then a bitcoin command has a network flag or a chain argument, such as : bitcoind -regtest currently you are presented with the mess
...

🚀 fanquake merged a pull request: "util: remove WSL 1 workaround in fs"
(https://github.com/bitcoin/bitcoin/pull/25898)

💬 fanquake commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1433021178)
@MarcoFalke merge or close?

💬 fanquake commented on pull request "build: Fail early and show actionable messages if autogen deps are missing":
(https://github.com/bitcoin/bitcoin/pull/25523#issuecomment-1433024510)
Thanks. However I don't think we are going to merge this. We'll also soon be migrating away from autotools & friends.

✅ fanquake closed a pull request: "build: Fail early and show actionable messages if autogen deps are missing"
(https://github.com/bitcoin/bitcoin/pull/25523)

💬 fanquake commented on pull request "Torcontrol opt check":
(https://github.com/bitcoin/bitcoin/pull/25136#issuecomment-1433026326)
@amadeuszpawlik are you planning on addressing any of the feedback here?

💬 Sjors commented on pull request "test: previous releases: add v24.0.1":
(https://github.com/bitcoin/bitcoin/pull/26586#issuecomment-1433037012)
tACK 741908afc1f9ed2040c18667c75665b300c5dfe7

I checked that it still works on top of current master. That said, there's no pressing reason to merge this. As we've done before, we could just leave the PR open and occasionally rebase it.


> apart from obvious smoke test issues that happen on every iteration, I don't think anyone even looks into failures here

This can also be a reason to not rush merging new releases. This PR itself is where most of the thorough checking happens, at ever
...

📝 Hyunhum opened a pull request: "Script explained"
(https://github.com/bitcoin/bitcoin/pull/27109)
While miniscript has a very kind explanation for it, original script does not.
I add simple explanation for the functionality of each opcode.
It will help a lot for developers who want to do script programming.

💬 Sjors commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1433040004)
Will rebase and address @darosior's comments soon(tm).

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108460621)
I looked into this again. The underlying problem seems to be that the kernel library still depends on the `ArgsManager`, which in turn depends on the global. I'd rather not make this change devolve into changing `ArgsManager` internals and behaviour, so I'll defer refactoring this global to once the `ArgsManager` is removed from the kernel library.

💬 furszy commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1433087966)
While I would like to merge this one (it's good as is), I think that would be better to check #25273 first. Mainly because this PR is about improving the `CoinControl` class documentation and #25273 modifies a big part of it (which, if we merge this one first, will make us re-do this doc improvements in another follow-up later).

💬 furszy commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1433091681)
@MarcoFalke, guess that Drahtbot shouldn't have requested review to me.

💬 MarcoFalke commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1433123977)
Yeah, if it was live 3 weeks ago, it would have requested a review from you before your comment. Which, I guess, would have made sense.

Also, it will only request reviews when there is an email notification sent out anyway, so the harm/risk should be minimal. But let me know if it should be tamed down or turned off.

💬 ajtowns commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108469843)
Seems more ergonomic to write:

```c++
CChainParams::SigNetOptions ReadSigNetArgs(const ArgsManager& args)
{
...
```

but maybe this is just deliberately keeping with the same pattern as per https://github.com/bitcoin/bitcoin/pull/26883/files#r1068247937

💬 ajtowns commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108499523)
Rename to `GetBuriedDeployment()` ? This function should be part of deploymentinfo.cpp, shouldn't it?

💬 ajtowns commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108500764)
`name` is really `deployment_id` (`dep` for short)?