💬 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`).

💬 furszy commented on pull request "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC":
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655812661)
> 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.

I'm not really strong here but It just seems a bit error-prone to always call to the `syncwithvalidationinterfacequeue` internally after bumping the scheduler timeout and not before. Existing call
...

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1277659822)
Yea, more elegant. Pushed changing it