💬 ryanofsky commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2717962615)
Also while the PR description mentions the `ENABLE_IPC` / `SANITIZERS=fuzzer`incompatibility to motivate this change, I think the change is a more general bugfix and makes sense for a variety of reasons listed previously in https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663597395:

> (1) It fixes the CI bug
> (2) It documents sanitize_interface and removes an attribute(weak) hack.
> (3) It doesn't compile libraries with a flag intended to bring in a main() function.
> (4) It pr
...

👍 hodlinator approved a pull request: "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets"
(https://github.com/bitcoin/bitcoin/pull/32019#pullrequestreview-2678590991)
re-ACK b86eb5f45a6df456d6aa80d62dd5b720ecfe917c

Would prefer something a bit more terse as mentioned in https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1988781317, but not a blocker for me.

📝 furszy converted_to_draft a pull request: "descriptors: inference process, do not return unparsable multisig descriptors"
(https://github.com/bitcoin/bitcoin/pull/31404)
The internal script-to-descriptor inference procedure shouldn't return invalid
descriptors or ones that fail the string-to-descriptor parsing process. These
two procedures should always support seamless roundtrips.

This inlines `InferScript`/`InferDescriptor` to the actual descriptors
`ParseScript` logic for the multisig path, ensuring only valid descriptors are
produced.

Examples where this presents an issue:
1) Because the legacy wallet allowed the import of invalid scripts, the pro
...

👍 hebasto approved a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2678573414)
ACK 564c54f3371b6faf50be2c7e7b6280357c90e62c.

Left a couple of nits and the following notes.

1. For some BSD systems, we document how to build BDB in depends. Now, it builds `sqlite` along with `bdb`. I don't believe it's critical, especially with BDB being removed soon.

2. re [comment](https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2717203847):
> I initially considered dropping the `NO_WALLET` option from depends. Most other depends packages refer to the library name rathe
...

💬 hebasto commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1991551292)
A corresponding change in `Makefile` may be done:
```diff
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -233,7 +233,6 @@ $(host_prefix)/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_$(fina
-e 's|@zmq_packages@|$(zmq_packages_)|' \
-e 's|@wallet_packages@|$(wallet_packages_)|' \
-e 's|@bdb_packages@|$(bdb_packages_)|' \
- -e 's|@sqlite_packages@|$(sqlite_packages_)|' \
-e 's|@usdt_packages@|$(usdt_packages_)|'
...

💬 hebasto commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1991555896)
This can be simplified:
```suggestion
wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages)
```

💬 theStack commented on pull request "correct wrong assumptions in the contrib linearize data script":
(https://github.com/bitcoin/bitcoin/pull/31888#issuecomment-2718026023)
Can you give more context on what problem this PR is trying to solve? Looking at the description and the diff I can't follow, e.g. it's unclear to me what an "incorrect open mode" is.

💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2718067110)
Hello @maflcko @furszy @fanquake
Can this be merged now?

💬 mprenditore commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2718070286)
@furszy @pablomartin4btc
Can this be merged or there's still something missing?

📝 soldate opened a pull request: "pull request test"
(https://github.com/bitcoin/bitcoin/pull/32040)
Testing

👍 ryanofsky approved a pull request: "Doc: add a comment referencing past vulnerability next to where it was fixed"
(https://github.com/bitcoin/bitcoin/pull/30538#pullrequestreview-2678760728)
Code review ACK eb0724f0dee307d6d14e47ebd3077b7ffd50f507. No changes since last review other than rebase

📝 soldate converted_to_draft a pull request: "pull request test"
(https://github.com/bitcoin/bitcoin/pull/32040)
Testing

✅ soldate closed a pull request: "pull request test"
(https://github.com/bitcoin/bitcoin/pull/32040)

📝 fanquake locked a pull request: "pull request test"
(https://github.com/bitcoin/bitcoin/pull/32040)
Testing

💬 m3dwards commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2718133961)
Built with and without depends and all was working

> I initially considered dropping the `NO_WALLET` option from depends. Most other depends packages refer to the library name rather than the feature, so this seemed better than dropping `NO_SQLITE`.
>
> However it could break automations (and habits) of anyone who tries to build the node without wallet support. So I ended up dropping `NO_SQLITE` and keeping `NO_WALLET`.

As someone who doesn't have those habits my vote would have been
...

💬 hodlinator commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2718134999)
Thanks for working on this!

> ... After running through the steps in this guide, you are encouraged to do your own testing.
>
> This can be as simple as testing the same features in this guide but trying it a different way. ...

These seem like they belong in the same paragraph? Either replace "This" with "Your own testing" or bring them together.

> introduces ephemeral dust to improve transaction packaging

Ephemeral dust transactions should be combined with a transaction that spends from the
...

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2718135064)
Pushed the two simplifications.

@hebasto the `NO_WALLET` option for depends has existed since at least v0.11: https://github.com/bitcoin/bitcoin/tree/v0.11.0/depends

📝 glozow opened a pull request: "build: bump CLIENT_VERSION_MAJOR to 29"
(https://github.com/bitcoin/bitcoin/pull/32041)

💬 glozow commented on pull request "build: bump CLIENT_VERSION_MAJOR to 29":
(https://github.com/bitcoin/bitcoin/pull/32041#issuecomment-2718194037)
Missed for branch-off, my bad. Thanks @fanquake

👍 fanquake approved a pull request: "build: bump CLIENT_VERSION_MAJOR to 29"
(https://github.com/bitcoin/bitcoin/pull/32041#pullrequestreview-2678861797)
ACK a3f0e9a4336a57440615efb352793fe131079487