📝 hebasto opened a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953)
On FreeBSD 14.0, in the `depends` directory:

- without `bash`:
```
$ gmake print-bdb_build_id_long
env: bash: No such file or directory
env: bash: No such file or directory
bdb_build_id_long=bdb-4.8.30-4b0c6f8e95251b9c6731844fc34111c04b75fd9f15c671d6e34f2a4d014ec1be-release
$ gmake print-final_build_id
env: bash: No such file or directory
env: bash: No such file or directory
final_build_id=722b2d3e264
```

- with `bash`:
```
$ gmake print-bdb_build_id_long
bdb_build_id_long=
...

💬 hebasto commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075088822)
cc @vasild @fanquake

💬 fanquake commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075094287)
This should be added to the depends docs.

💬 hebasto commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#issuecomment-2075104778)
> This should be added to the depends docs.

Thanks! Added.

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578006194)
Hm, ok, that's another behavior change I didn't anticipate. Let me first describe my understanding of happens: In [the libevent version](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3492) each of the two characters following the `%` are [looked at individually if they are valid hex characters](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3508). The space is not so the if fails and the fallback case takes
...

💬 fanquake commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#discussion_r1578008214)
So now it can be removed here.

💬 hebasto commented on pull request "doc: Bash is needed in gen_id and is not installed on FreeBSD by default":
(https://github.com/bitcoin/bitcoin/pull/29953#discussion_r1578012276)
Sure. Removed.

💬 achow101 commented on issue "Calling `walletprocesspsbt` to sign multisig containing `OP_GREATERTHANOREQUAL` op_code":
(https://github.com/bitcoin/bitcoin/issues/29949#issuecomment-2075118310)
Bitcoin Core cannot sign non-standard scripts or scripts that are not Miniscript. This is expected behavior. `walletprocesspsbt` does not return an error because signing is not the only thing that it does, so returning an error for a failing to sign is not appropriate.

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578019052)
The hex checking is done inside of from_chars (locale independent), so you can re-use that. Something like this may work:

```suggestion
if (c == '%' && i + 2 < url_encoded.size()

) {
int decoded_value{0};
auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

if(ec == std::errc{} && p==url_encoded.data() + i + 3) { // hex

res += static_cast
...

🚀 fanquake merged a pull request: "ci: disable `_FORTIFY_SOURCE` with MSAN"
(https://github.com/bitcoin/bitcoin/pull/29837)

💬 sipa commented on issue "Calling `walletprocesspsbt` to sign multisig containing `OP_GREATERTHANOREQUAL` op_code":
(https://github.com/bitcoin/bitcoin/issues/29949#issuecomment-2075135656)
For some more background, Bitcoin Core has logic to try various things when signing, which includes Miniscript. The script variant you tried with `OP_NUMEQUAL` at the end is valid Miniscript (`multi_a(2,<key1>,<key2>,<key3>)` specifically), but the variant with `OP_GREATERTHANOREQUAL` is not.

When signing fails, Bitcoin Core doesn't know why it failed; it simply didn't find any patterns it knew how to sign for, and as @achow101 points out, it doesn't even know your intent was to sign.

🚀 fanquake merged a pull request: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)

👍 alfonsoromanz approved a pull request: "test: Assumeutxo: snapshots with less work should not be loaded"
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2020129687)
Code review and tested ACK 4922f56660ae7834598c6dc4904a6711c23edfa3

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578065722)
Yepp, looks good, thanks!

🚀 glozow merged a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757)

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2012191090)
Updated 974b16b2580f4f09c8936fbead96fdbee637cb14 -> c45cb13b9e4f6eae50ab6dc42acf471921980591 ([`pr/saferesult.1`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.1) -> [`pr/saferesult.2`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.1..pr/saferesult.2)) with suggestions

re: https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2011883406

> I'm unsure about removing the assignment ope
...

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817468)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572639146

> nit: nice, but maybe best to clean up all 3 instances in one go?

Thanks! Applied patch

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817364)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572648440

> `Update` is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think `Replace` would be better. For the next commits in #25665 where we actually add the merging functionality, I think `Merge` or `Combine` would be a better choice.

We could definitely have a `Replace` method
...

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817536)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572632242

> nit: Any reason not to just wrap the args in `std::move`? Seems like the more intuitive approach, and probably slightly more performant?

Agreed that's much better. Applied suggestion.

👍 maflcko approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2020255232)
lgtm, but `unsigned` may be better