💬 hebasto commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431519355)
@fanquake

> 3\. [Comments like](https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1425962907):
> > When are we removing Autotools?
>
>
> > > Any time developers are comfortable with.
>
>
> are pretty hand wavy, and do not establish any sort of plan.

My comment is "pretty hand wavy" for the only reason -- I have no experience of introducing a new build system for a project like Bitcoin Core (#2943 happened before I ran my first Bitcoin Co
...

💬 hebasto commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1107255177)
> ... not sure what documentation you would suggest adding in this case?

The PR description? A commit message?

💬 hebasto commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1107257611)
I'm curious now, why functional tests did not catch this change?

💬 sipa commented on issue "Disallow duplicate leaves inside `tr()` descriptors":
(https://github.com/bitcoin/bitcoin/issues/27104#issuecomment-1431533351)
I think it'd be useful to provide some mechanism to detect duplicated leaves, and possibly a way to discourage/warn about their usage. I'm not sure that outlawing such descriptors in the first place is a good idea.

E.g. could we add a (generic) mechanism to descriptors to mark silly/dangerous/... constructions, and e.g. prevent them from being imported in wallets (except maybe with a "force" argument), or prevent them from being used in `deriveaddresses`. This mechanism could perhaps also be
...

💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1107266264)
done

💬 mzumsande commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1431541515)
> Please let us know if you want to get this merged or address them.

I'll address the comments later this week!

👍 MarcoFalke approved a pull request: "test: simplify and speedup mempool_updatefromblock.py by using MiniWallet"
(https://github.com/bitcoin/bitcoin/pull/27035)

🚀 MarcoFalke merged a pull request: "test: simplify and speedup mempool_updatefromblock.py by using MiniWallet"
(https://github.com/bitcoin/bitcoin/pull/27035)

👍 hebasto approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 pinheadmz commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1107304905)
done, thanks

👍 furszy approved a pull request: "refactor: Disable unused special members functions in `UnlockContext`"
(https://github.com/bitcoin-core/gui/pull/711)

📝 MarcoFalke converted_to_draft a pull request: "ci: Use dedicated root path for ci"
(https://github.com/bitcoin/bitcoin/pull/27090)
Now that stuff (like depends) is no longer propagated back to the host
folder after commit 141115a0604001b8cce848804798dc174770a994, commit
85892f77c98c7a08834a06d52af3eb474275afd8 can be reverted and
`LOCAL_USER` removed.

So do that. Also for clarity, in the same commit, use a dedicated path
for the ci root folder `CI_ROOT_DIR`.

💬 TheCharlatan commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1107320759)
In this case why not decouple the two options and pass and construct them independently? I've been using this branch for further work on getting the gArgs out of blockstorage.cpp and got the impression like there is little benefit in nesting the options.

🚀 fanquake merged a pull request: "Net: Pass `MSG_MORE` flag when sending non-final network messages (round 2)"
(https://github.com/bitcoin/bitcoin/pull/26844)

👍 TheCharlatan approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 fanquake commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#issuecomment-1431625966)
@achow101 can you take a look here

👍 ponury1990 approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 achow101 commented on pull request "Remove laanwj from trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27054#issuecomment-1431661196)
ACK aafa5e945cef7a4f65ddadcf548932dd4e27ada1

💬 achow101 commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1107398449)
In e86214692d3fe7a8c835e07f7ebd47a9afb9fec9 "wallet: set keypool_size instead of access global args manager"

The `using ScriptPubKeyMan::ScriptPubKeyMan;` line above should be removed so that this new constructor is the only constructor to avoid possible programming errors where the keypool size is not passed in.

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1431696802)
`7a93d7b0f9...8991ed2c6e`: address suggestions