💬 Christewart commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3572313467)
> > Hi AJ, are these the test vectors you had in mind?
>
> I was thinking functional tests, to also make sure there isn't some obscure policy rule that the unit tests miss that nevertheless ends up blocking successful spends of things that should be spendable. Unit tests are good too though.

:+1:

> The things that (per this PR's arguments) should be spendable are `1 <pubkey> <invalid> 2 CMS` and `1 <invalid> <pubkey> 2 CMS`.

I took a closer look through `script_tests.json` and it see
...

🤔 mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3501853880)
re-ACK 8343a9ffcc752f77eb2248315d10b6dff4a5c98b

💬 frankomosh commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#discussion_r2557421280)
Thanks for suggestion. Can we keep it this way for now?

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3572366307)
@willcl-ark's [suggestion](https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2556213666) has been taken.

Thank you!

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557502178)
@frankomosh should be addressed

💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557503481)
@optout21 for vision

🤔 l0rinc requested changes to a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3497639202)
Thank you for considering the previous changes.

Something kept bothering me in the usage of sorted sets, how awkward it was to use them as changing priority queues, so I stepped back and checked why we even need such an optimization in the first place, given don't expect to have millions of entries here.

I have included a benchmark in my suggestions to back my opinion with actual data, please consider it, and feel free to add any part to the current PR if you agree that it simplifies the logic
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557559349)
As mentioned above, this hasher is quite heavy, making this a pessimization:

<img width="823" height="411" alt="Image" src="https://github.com/user-attachments/assets/7b8ca62e-6e99-40fd-ad39-5b81e1b02f1c" />

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556376564)
As the [developer notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes) hint at, this probably isn't fixed in [*16.2*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [*16.3*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).

And it seems https://github.com/bitcoin/bitcoin/pull/33932 doesn't change the situation since we're [deliberately downloading](https://git
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556353522)
I like the new version, but I want to give the [extract](https://en.cppreference.com/w/cpp/container/map/extract) another try, see if you like it more - if you don't please resolve this, no explanation needed.

With erase+emplace you destroy the node and construct a new one, with extract you detach the existing tree node, mutate the key in the detached node, reinsert the same node. It's not a huge difference, but if there's a dedicated tool for it since C++17, why not use it, it expressed inte
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556829022)
Forgot to ask: what does the `/` mean in `then we consider it stale / for rebroadcasting`?

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556560989)
isn't there an asymmetry between `PrivateBroadcast::Add` where we are skipping adding by `tx->GetHash()` only, but not removing if `by_txid->second.tx->GetWitnessHash() == tx->GetWitnessHash()`?
https://github.com/bitcoin/bitcoin/blob/7826148b12048a40919061d935b1abb2cd49dcb3/src/private_broadcast.cpp#L16-L17

The tests seem to be failing if I check wtxid for insertion as well...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556667714)
> Hmm, is this a general frowning against size_t?

From my side it is, I don't want to think about architecture for code that's unrelated to it.

> My thinking is that CPUs work best with word sized integers

Making it smaller gives the CPU more information, not less, so it can still revert to storing it aligned on more space (the opposite isn't really possible if we already define it as max size)

> 64 bit integers on 32 bit CPUs will bloat the program unnecessary and 32 bit integers on
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553965873)
nit: seems to be copied from somewhere else:
```suggestion
# Copyright (c) 2023-present The Bitcoin Core developers
```

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682639)
[78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682926)
Sounds good -> [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557683518)
Thanks!
Fixed in [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0).

💬 enirox001 commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3572670833)
tACK c0bfe72

Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.

The switch to string_view correctly handles string literal null terminators.

🤔 enirox001 reviewed a pull request: "Change Parse descriptor argument to string_view"
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3502191427)
tACK c0bfe72

Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.

The switch to string_view correctly handles string literal null terminators.

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3572692780)
rfm?