💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598094935)
Using `IsDust()` makes no different for the regular index: https://github.com/Sjors/bitcoin/commit/20aba5dfa34224432787ac450b8e9d3a66d8eea9

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598099088)
Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2107001990)
Fixed the Windows build error, but drafted while based on #30078.

📝 fanquake converted_to_draft a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723)
Based on #30078.

This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:
* https://github.com/zeromq/libzmq/pull/4668
* https://github.com/zeromq/libzmq/pull/4670

and another Windows-related change:
* https://github.com/zeromq/libzmq/pull/4678

I also noticed the CMake Windows version autodetction was broken with mingw-w64 (https://github.com/zeromq/libzmq/issues/4669), so we set the version explicitly.

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598105196)
Would prefer to not change for now to keep this a move-only commit.

💬 fanquake commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598107518)
Should remove `-DHARDENING=OFF` here.

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179)
I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.

💬 ajtowns commented on issue "Compilation failure with `-O0` + `-fsanitize=address` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2107018954)
How about adding something like:

```diff
--- a/configure.ac
+++ b/configure.ac
@@ -349,7 +349,14 @@ esac
if test "$enable_debug" = "yes"; then

dnl Disable all optimizations
- AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"], [], [$CXXFLAG_WERROR])
+ case "$use_sanitizers" in
+ *address*)
+ AX_CHECK_COMPILE_FLAG([-Og], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"], [], [$CXXFLAG_WERROR])
+ ;;
+ *)
+ AX_CHECK_COMPILE_FLAG([-O0], [DEBUG_CXXFL
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598119960)
I don't really see a benefit. We shouldn't be exposing this value to anything outside the class, so defining a constexpr here seems unnecessarily verbose.

💬 ajtowns commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2107073436)
> So the 2:30 block times are expected then, and at the first difficulty adjustment I would be expecting the difficulty to increase to such a point that it would take my CPU 600 seconds to generate a block.

I think you've got NBITS=1d00ac8d, which if my math is correct is ~1300 times more difficult than the min signet difficulty, which should take ~5 retarget periods to achieve. The 6th one would run at about 7m46s per block; for a total of ~28 days to ramp up the difficulty. (If you backdate
...

💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2107074521)
utACK d7290d662f494503f28e087dd728b492c0bb2c5f

✅ remyers closed a pull request: "wallet: Target a pre-defined utxo set composition by adjusting change outputs"
(https://github.com/bitcoin/bitcoin/pull/29442)

💬 remyers commented on pull request "wallet: Target a pre-defined utxo set composition by adjusting change outputs":
(https://github.com/bitcoin/bitcoin/pull/29442#issuecomment-2107079658)
Replaced by simpler #30080

👍 josibake approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2052111060)
ACK https://github.com/bitcoin/bitcoin/pull/26606/commits/a0943b812ef38826a4ee2732af5f24e470281117

Spent some time comparing this to the bdb header files and LGTM. Test coverage also looks good. Left a few nits regarding comments. Overall, I do think we should prompt users to file an issue specifically for this if loading a database fails, considering it might be nothing wrong with their database and instead an edge case being hit in the parser.

💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598149611)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/e643cac230fc2ba059847bd4e196bddfd97b1e8e ("Add MakeBerkeleyRODatabase"):

Seems like we should have some sort of call to action here prompting the user to open an issue / reach out on IRC if this fails? If we fail to open a bdb database here, the database itself could be fine and it instead might be an issue with our parser.

💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598137085)
in https://github.com/bitcoin/bitcoin/pull/26606/commits/5f0cddb4f66f2a174babdfb17bc31a3011e5cc20 ("wallet: implement independent BDB deserializer in BerkeleyRODatabase"):

nit: I somewhat know whats going here on based on previous discussions about this PR, but without that prior context this being commented out is pretty confusing and I don't think the comment above provides enough explanation. Perhaps add more context in the comment or just remove this snippet entirely?

💬 josibake commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1598126028)
nit: might be worth adding a comment here explaining this. I was also surprised to see this not returning `false` and couldn't understand why this was different from other methods until I read your comment here.

👍 paplorinc approved a pull request: "refactor: Model the bech32 charlimit as an Enum"
(https://github.com/bitcoin/bitcoin/pull/30047#pullrequestreview-2052206587)
nice!

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598184096)
nit: to be consistent with the casing below
```suggestion
/** Character limits for Bech32(m) encoded strings. Character limits are how we provide error location guarantees.
```

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598185043)
nit:
```suggestion
BECH32 = 90, //!< BIP173/350 imposed a 90 character limit on Bech32(m) encoded addresses. This guarantees finding up to 4 errors.
```