💬 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

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578146669)
style nit: snake_case for new code, and newline before `{`, according to clang-format

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578133898)
This must be `unsinged`, otherwise a sign (`-`) is treated as valid hex

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578145369)
style nit: The verbose member calls can be avoided by turning `i` into an iterator:

```diff
std::string urlDecode(std::string_view urlEncoded) {
std::string res;
res.reserve(urlEncoded.size());

- for (size_t i = 0; i < urlEncoded.size(); ++i) {
- char c = urlEncoded[i];
+ for (auto i = urlEncoded.begin(); i < urlEncoded.end(); ++i) {
+ char c = *i;
// Special handling for percent which should be followed by two hex digits
// represe
...

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578134591)
`%-1`

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578118369)
style nit in fcee97601f464e3330d10ff85e19e6ce17452897: Missing space, according to clang-format.

💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1578168924)
Thank you for the review!

Great question. The location chosen was within the `# vv Tests less than 30s vv` portion of `BASE_SCRIPTS`. I'm seeing run-times for `feature_framework_unit_tests.py` (on two machines I'm using) of 9s and 6s, and `TEST_FRAMEWORK_UNIT_TESTS` is around mid-way in the "30s" portion of the list, which seemed reasonable.
https://github.com/bitcoin/bitcoin/blob/f19f0a2e5af6c2a64900f1f229e21b6f1668bd3d/test/functional/test_runner.py#L392-L393

The existing comment to i
...

🤔 theStack reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2019874263)
Another round through (modulo the fuzz commit), the code looks correct to me, just left a few nits below. Planning to do another review tomorrow with focus on the main commit bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6, especially going through the `AlreadyHaveTx` call-sites and their `include_reconsiderable` values to check once more.

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577902195)
nit: for the sake of completeness, could also check that txid and wtxid are equal for non-segwit tx 2:
```suggestion
BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
BOOST_CHECK_EQUAL(txid_2.ToUint256(), wtxid_2.ToUint256());
BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());
```