👍 theStack approved a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1551988560)
ACK 1c7582ead6e1119899922041c1af2b4169b0bc74

Finished reviewing. Code looks good to me, checked carefully that the implementation matches BIP324 and verified the test vectors (see above). :heavy_check_mark:

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1655594515)
@vostrnad's comment makes sense to me. I'll hold off re-reviewing until that's either done or decided against.

💬 Sjors commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#issuecomment-1655597008)
> If #17487 gets merged I will rebase this

It has been merged.

💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277494461)
That's a large increase in runtime isn't it? Won't 100k iterations take much less than 100 minutes to complete on most of our targets?

We've got 115 targets at the moments, this change makes it so running this script with `--generate` for all targets would take around 8 days.

💬 pinheadmz commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1655625669)
hint: https://github.com/bitcoin/bitcoin/blob/42a9110899a9020417f0397456168aea6ac6ade9/test/functional/feature_taproot.py#L989

💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277507602)
The number of tasks that are spawned is ~300 (`294` for me, but it will likely be more in the future). So even if you run each task only for a minute, it will take longer than 4 hours already. But running for a minute seems entirely useless, so I am not sure if it is worth optimizing for. (For smoke testing I just edit this script to `-max_total_time=6`).

In any way, you can pass `--par 999` to run everything at the same time and be done in 100 minutes.

However, to avoid an edit for smoke
...

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1655662987)
ACK 91f47e676ae3f131e7cecae9b5b5e50b358e9b32

Also tested that reverting 6a9510d2dabde1c9ee6c4226e3d16ca32eb48ac5 causes the test to fail when it tries to load the wallet in master after having loaded it in v25 (`Wallet corrupted`).

💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277548417)
Ok i'm confused. I'm aware i just gave this a superficial look but i'm absolutely not following here. *(Last comment and if i don't get it i'll look into it more in depth.)*

> The number of tasks that are spawned is ~300

How comes? The default for `--par` is `4`.

> So even if you run each task only for a minute, it will take longer than 4 hours already

How could, with 300 tasks, running 169 tasks for 1 minute take 4 hours?

----

My main worry would be that increasing the runtime
...

💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277559716)
> How comes?

Sorry, with "The number of tasks that are spawned is ~300" I meant "The number of tasks that are spawned over the full runtime of the python script is ~300".

> How could

For example, with `-j1`: Running 300 1-minute-tasks spawned sequentially on one thread will take 300 minutes. (5 hours). Though, the main point is that 1 minute of runtime is useless. So optimizing for a short default runtime will quickly get you to make the entire script useless.

I am happy to change th
...

💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277560378)
see above.

better boolean notation here would be great.

💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277557756)

`pop two` doesn't quite work (IMO).

it makes the order somewhat ambiguous.

for example:

`pop pop add push` is more correct. no?

Being more explicit in this way will make the `OP_SUB` make more sense, no?.

`pop pop subtract push`


for `OP_ADD` it isn't much of an issue.

`` 1 + 1 = 2 ``

`` 1 + -1 = 0 ``

`` -1 + 1 = 0 ``




for `OP_SUB` it may be an issue.

``1 - 1 = 0``

``1 - -1 = 0``

``-1 - 1 = -2``

*in common arithmetic notation

💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1655711887)
Concept ACK

💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277566436)
Indeed a too short runtime could make the script useless (since people would basically start running it in a loop itself), but a too long one would likely introduce a bias toward the first (alphabetically sorted) targets.

I guess it would be helpful to know who uses this script and how. For instance if they are continuously generating coverage most likely they are already running it in a loop of some sort and therefore a runtime of like 10minutes per target would spead the time spent over all
...

💬 brunoerg commented on pull request "fuzz: use `ConnmanTestMsg` in `connman`":
(https://github.com/bitcoin/bitcoin/pull/28091#issuecomment-1655714812)
friendly ping: @dergoegge

👍 darosior approved a pull request: "fuzz: Generate with random libFuzzer settings"
(https://github.com/bitcoin/bitcoin/pull/28178#pullrequestreview-1552162689)
utACK fad32eb02c98d841e0cd9b585b61a698feec361a. I'm not using this script myself but the changes make sense to me.

💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277568660)
Nice, do you happen to know why they don't only set it to `1` when `-jobs` is not `0`?

💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277575619)
It is possible to launch multiple libFuzzer *manually* on the same folder, but this script doesn't do it, and it seems unlikely that a user would do it.

💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277576883)
@murchandamus may be using it ?

🤔 furszy reviewed a pull request: "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC"
(https://github.com/bitcoin/bitcoin/pull/28118#pullrequestreview-1552181867)
Little topic, mainly to not mix RPC commands responsibilities.

Why not implementing this on the test framework instead?

Could add a proxy function `mockscheduler` to `test_node.py` that calls to the real `mockscheduler` and, subsequently to `syncwithvalidationinterfacequeue`.

💬 Ayms commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1277595211)
Can you please explain why ?