💬 maflcko commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2662243280)
Closing for now, due to lack of progress for more than a year. Please leave a comment, if you want this reopened. Anyone may also open a new pull request, referencing this one, and including a short summary of any relevant discussion comments from here.

👍 maflcko approved a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2620206804)
lgtm. Seems fine to add an option to say "I don't have python and I don't care about the tests"

💬 maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#discussion_r1957725461)
shouldn't this be an error now?

💬 maflcko commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1957729211)
https://github.com/bitcoin/bitcoin/issues/31881

💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957746797)
> expressed seeing value in reserving `assert` for internal sanity checks in the tests

Is there an example of an internal sanity check? How would that be different from the functional framework unit tests? Also, why would there be any risk in treating an internal test error as a "normal" test error?

I understand the distinction of internal errors in user-facing programs, but for dev-only test-only scripts, the meaning and the tradeoffs become less clear.

💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957749674)
Another review note, it seems like `assert_true("abc")` will pass. This seems confusing and I am also not sure if it is clearer than `assert "abc"`. (Same for integral values like `3`, ...)

And on a general note, the attempt here seems similar or related to the discussion (and changes) stemming from https://github.com/bitcoin/bitcoin/issues/23119. However that issue is stale for more than 3 years, with no meaningful progress, so I wonder how important it is and whether it can be closed.

💬 maflcko commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957757389)
I think further changes were left for a follow-up, see https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550:

> Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2662344876)
> Does it happen with a smaller (default) dbcache?

Yes, it does happen with default settings, including default value 450 of dbcache.

💬 TheCharlatan commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662351232)
Re https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660002068

> > Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
>
> No, these should all be codesigned now.

and re https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662230723

> On Apple Silicon downloading through Safari and then extracting in Finder fails:
> ...
> Similarly on Intel
...

💬 TheCharlatan commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#issuecomment-2662391575)
> I would be a fan of going further and making the BlockManagerOptions ctor take a BlockDirLock (or, as per my comment a DirectoryLock) to allow more granular error feedback, but this is already a step in the right direction regardless.

I attempted implementing this, but giving the options the lock starts to introduce non-trivial additional semantics to the options: Since the lock needs to be moved from the options to the block manager, the options cannot be re-used. I'm not sure how to imple
...

💬 maflcko commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2662416943)
> Eagerly awaiting C++26's explicit `[[indeterminate]]` [initialization](https://en.cppreference.com/w/cpp/language/attributes/indeterminate).

Maybe I am missing something, but I don't think `[[indeterminate]]` should be used here either. My understanding is that `[[indeterminate]]` in C++26 is simply a way to restore pre-C++-26 UB. However, if there really is UB in one of those functions, we'd want the C++26 erroneous behavior so that the implementation is encouraged to issue a diagnostic (a
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2662441920)
`e1671ff42c...741f17e51d`: rebase and remove the first commit which was merged via https://github.com/bitcoin/bitcoin/pull/31854, thanks!

💬 maflcko commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712)
how could a fuzz engine work around the checksum? I guess it may be small enough to allow it to rarely solve the checksum. However, given the removed unit test, I am not sure if the coverage is identical.

It would be good to submit the inputs from the unit test as fuzz inputs, as well as any fuzz inputs that got stale by renaming/splitting the fuzz target. See also:

```
[06:22:44.212] Fuzzing harnesses lacking a corpus: base32_encode_decode base58_encode_decode base58check_encode_decode
...

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883107)
nit: missing trailing comma.

Also, what is the point of combining them, when they are handled completely separate anyway?

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883443)
nit: please use `std::span` for new code

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957893638)
why remove this call?

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662571593)
This is an interesting writeup [Hurdles of macOS distribution](https://gist.github.com/rsms/929c9c2fec231f0cf843a1a746a416f5). It suggests we shouldn't use `--deep`.

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662667293)
It might be worth trying to notarize the binaries (instead of only the GUI bundle). This [forum thread](https://forums.developer.apple.com/forums/thread/721117) suggests it can be done by temporarily zipping them and sending them Apple in that form.

(maybe in a followup)

💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2662789145)
rfm?

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958078713)
There is no need to remove it, will put back. Probably missed it during the rework. Thanks.