💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107161592)
nit in cf307244db97e7b9e29a3d0ff948a6fa7877b548: In the future it could make sense to return the error right away for better UX? But would probably be too much for this pull here? :man_shrugging:

💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107159246)
nit in 65067ee3d9730b9a121ed36d6a720cc7530a5d77: Could check that the return value is now `false`, instead of `true`?

💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107162432)
nit in cf307244db97e7b9e29a3d0ff948a6fa7877b548, if you retouch: clang-format?


```diff
diff --git a/src/node/chainstate.h b/src/node/chainstate.h
index db8b4f945e..1e885a22f1 100644
--- a/src/node/chainstate.h
+++ b/src/node/chainstate.h
@@ -36,11 +36,12 @@ struct ChainstateLoadOptions {
//! case, and treat other cases as errors. More complex applications may want to
//! try reindexing in the generic failure case, and pass an interrupt callback
//! and exit cleanly in the interru
...

💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107164412)
nit in 42b192f0cbf9c04da111145c921344b0881b3ce3:
```suggestion
dbcache, Bitcoin Core will now return an error at startup. (#25574)
```

(holds for bitcoin-qt as well)

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

💬 hebasto commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1107165674)
nanonit: slightly prefer
```suggestion
walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1});
```

as it was before, and is used recently in our code base.

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

⚠️ darosior opened an issue: "Disallow duplicate leaves inside `tr()` descriptors"
(https://github.com/bitcoin/bitcoin/issues/27104)
While working on adapting Miniscript to Tapscript, i [noticed](https://gist.github.com/sipa/06c5c844df155d4e5044c2c8cac9c05e?permalink_comment_id=4434770#gistcomment-4434770) signatures in Tapscript only commit to the leaf (i previously assumed they committed to the whole branch). It's been discussed lately on the mailing list as well ([1](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-February/021452.html), [2](https://github.com/bitcoin-inquisition/bitcoin/issues/19)). This mean
...

💬 darosior commented on issue "Disallow duplicate leaves inside `tr()` descriptors":
(https://github.com/bitcoin/bitcoin/issues/27104#issuecomment-1431453521)
In the case of the Bitcoin Core wallet it would be a backward incompatible change though..

💬 glozow commented on pull request "verify-commits: Bump trusted git root to after most recent laanwj merge":
(https://github.com/bitcoin/bitcoin/pull/27076#issuecomment-1431494681)
post merge ACK 6ada37d44cce7fa3a729de72cede4c1f3bc675ce

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1107231839)
Right that's a good catch. We still _behave_ the same as previously, either fetching `conf` from an absolute path or completing the path with default datadir, but now because I simplified [reading the conf into a stream](https://github.com/bitcoin/bitcoin/pull/27073/commits/6207b73e508d6cfc7583483d00261c66b0c348c3#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L993), we return the full path we were trying in the error.

I agree that therefore there is a slight behaviour c
...

🚀 fanquake merged a pull request: "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements"
(https://github.com/bitcoin/bitcoin/pull/26153)

💬 fanquake commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#issuecomment-1431505288)
Going to move this forward. @real-or-random would still be great for you to leave any post-merge commentary, if you get the time.

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