💬 glozow commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1705470107)
I think the previous text is clear and not inaccurate, so I'm wondering whether this last commit is necessary.

💬 darosior commented on issue "Unexpected behaviour when using `sortedmulti_a` descriptor":
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271210770)
I'm not sure. `sortedmulti_a` was introduced for compatibility with legacy pre-descriptor practices. I can't think of any reason to ever prefer `sortedmulti_a`/`sortedmulti` to `multi_a`/`multi` inside Miniscript. And there is no legacy compatibility concern since any Miniscript-using wallet must have been created in a post-descriptor world.

So i don't think it's worth adding `sorted*` fragments to Miniscript.

👍 TheCharlatan approved a pull request: "build: Remove unused visibility checks"
(https://github.com/bitcoin/bitcoin/pull/30590#pullrequestreview-2221207346)
ACK bbcba09cd5ca5fdd9055aaf64781125c5e505576

The `visibility("default")` attribute is still used in `src/tinyformat.h`, but I don't think we need this check for that, since it is already gated.

💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705483297)
IIUC, I don't think static makes a difference here since we aren't talking about a class member? Regarding `CHARSET` and `CHARSET_REV`, what's the advantage of having them in the header file if we aren't using them outside of bech32.cpp?

💬 fanquake commented on pull request "build: Remove unused visibility checks":
(https://github.com/bitcoin/bitcoin/pull/30590#issuecomment-2271230120)
> but I don't think we need this check for that, since it is already gated.

Yea. I think for our use, that (and related) code could also just be removed entirely.

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705520136)
> I don't think static makes a difference here

Based on other usages in the code, `static constexpr` in headers is good practice to prevent potential linking issues and ensure each translation unit has its own independent copy. Let me know if I'm wrong here.

> what's the advantage of having them in the header file

Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.

If you think this is outside the scope, I can accept
...

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#issuecomment-2271270916)
ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

💬 brunoerg commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2271271424)
Concept ACK

💬 willcl-ark commented on pull request "rename TransactionError:ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2271278295)
* Dropped the first commit based on feedback recieved.
* Dropped the final commit, as following merge of #29855, I don't think that we can actually hit a `PSBTError::MISSING_INPUTS` any more, as we return early. #29855 also modified the only test we had for a `MISSING_INPUTS` error, and I can't trivially see a codepath to trigger it any more. Will address this in a followup.

🤔 marcofleon reviewed a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596#pullrequestreview-2221309312)
ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d. Ran the test a bit to be sure, lgtm.

💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2271296632)
Agree with @petertodd’s assessment: I don’t think this should be included in v28.0. It might be nice to issue a deprecation warning in v28 on the startup parameter, though.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705542508)
Good idea, updated `fuzz/hex.cpp` to cover `result_size` with an `int16_t`. Not sure what the optimal range of values would be here, but I figured not making it too large (`int`) makes sense given that `result_size` is not user input?

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2271300910)
@murchandamus feel free to open the deprecation PR :+1: I'm leaving as draft as noted in OP until post-28

💬 willcl-ark commented on issue "docs: Windows build intructions result in a large binary":
(https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2271304490)
In a post-cmake world this is the type of thing I would expect developers to add to their own [cmakuserpresets.json](https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html), although I don't see any reason it couldn't be documented too. Compiling without debug symbols is already documented in build-unix.md:

https://github.com/bitcoin/bitcoin/blob/43740f4971f45cd5499470b6a085b3ecd8b96d28/doc/build-unix.md?plain=1#L30

But even building with `CFLAGS` and `CXXFLAGS` leaves some info `
...

💬 ismaelsadeeq commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1705571908)
Thanks I removed the last commit to focus this PR on #30275 follow-up.

🤔 glozow reviewed a pull request: "doc, rpc : `#30275` followups"
(https://github.com/bitcoin/bitcoin/pull/30525#pullrequestreview-2221386015)
ACK fa2f26960ee084971ab09959b213a9b8104482e5

👍 brunoerg approved a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596#pullrequestreview-2221391468)
utACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

🚀 glozow merged a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596)

💬 sipa commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705601751)
@josibake The keyword `static` in C++ has two completely different meanings sadly :angry:

Inside a class, a `static` variable/function makes it a class member rather than an instance member (shared by all instances).

Outside a class, `static` means that a variable/function is only accessible within its compilation unit (whose effect is identical to placing it in an anonymous `namespace`).

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705621067)
I managed to make it work, I think the internal IDE setting were overwriting the ones I was setting from the command line.

For the record, with these settings the bench and fuzz projects are enabled (though no IDE assistance - i.e. run from the IDE -, but that's a different issue):
<img src="https://github.com/user-attachments/assets/06b10237-c4b3-4ca3-8547-b3299d214070">

I did open an unrelated suggestion for better error messages: https://github.com/hebasto/bitcoin/issues/306