💬 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 ?

💬 MarcoFalke commented on pull request "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC":
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655764715)
> Why not implementing this on the test framework instead?

That just seems brittle, and bloated, for no reason? It is brittle because one may miss to implement a proxy, as they'll have to be done for the bitcoin-cli utility and the python rpc wrapper, and devs can still accidentally sidestep it. It is bloated because it will be more than one line of code.

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277557586)
0ce805b632dcb98944a931f758f76f530f5ce5f2: why isn't `BLOCK_VALID_TRANSACTIONS` also alternatively `ASSUMED_VALID`?

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277540196)
0ce805b632dcb98944a931f758f76f530f5ce5f2: why not `less than BLOCK_VALID_TRANSACTIONS`? Since the minimum requirement to download an assumed-valid block is that we have its headers, i.e. we're at `BLOCK_VALID_TREE` or above.

Same question for `RaiseValidity`, which this PR doesn't touch. Contrast that to `CheckBlockIndex()` which skips checks for `BLOCK_VALID_TRANSACTIONS`, `BLOCK_VALID_CHAIN` and `BLOCK_VALID_SCRIPTS` when assume valid is set.

I tried changing `BLOCK_VALID_SCRIPTS` to `BL
...

🤔 Sjors reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552110152)
The documentation commit made me wonder if we can use `BLOCK_VALID_SCRIPTS` (the first check beyond headers) instead of `BLOCK_VALID_TRANSACTIONS` to set `BLOCK_ASSUMED_VALID`.

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277559238)
0ce805b632dcb98944a931f758f76f530f5ce5f2: maybe make this:

```cpp
//! All (non-assumed) validity bits.
```

Which makes functions that use this more readable:

<img width="659" alt="Scherm­afbeelding 2023-07-28 om 15 44 16" src="https://github.com/bitcoin/bitcoin/assets/10217/2b309b0c-ac4a-42ff-9257-03cb75c057ee">

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655802652)
Pulled in the new changes, will update for exceptions shortly.

💬 mzumsande commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277639934)
> why not `less than BLOCK_VALID_TRANSACTIONS`?

I asked myself the same question recently and came to the conclusion that it's because when we finally end up downloading the assumed-valid blocks, there will be a period in time where we've saved the block (and therefore raised its validity to `BLOCK_VALID_TRANSACTIONS`) but haven't connected it yet to the background chainstate (so it's still `ASSUMED_VALID`).