💬 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.
```

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598199292)
Keeping it present tense feels more correct, perhaps:

```suggestion
BECH32 = 90, //!< BIP173/350 imposed character limit for Bech32(m) encoded addresses. This guarantees finding up to 4 errors.
```
?

📝 paplorinc opened a pull request: "Optimize memory allocation for transaction outputs"
(https://github.com/bitcoin/bitcoin/pull/30093)
Added a reserve operation to `txNew.vout` to pre-allocate memory based on the expected number of transaction outputs.

Split out of https://github.com/bitcoin/bitcoin/pull/30050/files#r1597631104

🤔 TheCharlatan reviewed a pull request: "fuzz: a new target for the coins database"
(https://github.com/bitcoin/bitcoin/pull/28216#pullrequestreview-2052152342)
lgtm, but I think it would be good to rebase this so the CI can run through it again.

💬 TheCharlatan commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1598150932)
Unrelated to this PR, but is there a reason this is using the full testing setup?
```diff
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index 8f3e357a84..c7563ae9f4 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -30 +30 @@ namespace {
-const TestingSetup* g_setup;
+const BasicTestingSetup* g_setup;
@@ -42 +42 @@ void initialize_coins_view()
- static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+
...

💬 paplorinc commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598202690)
Split it out to a new PR so that we don't forget it: https://github.com/bitcoin/bitcoin/pull/30093/files

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598204445)
Even better!

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598210040)
Filtering < 546 dust makes no difference on the regular index size (https://github.com/bitcoin/bitcoin/commit/411f035f0e09142311b6a829490f175582b07096)