💬 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.
(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 ?
(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`.
(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 ?
(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.
(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`?
(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
...
(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`.
(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="Schermafbeelding 2023-07-28 om 15 44 16" src="https://github.com/bitcoin/bitcoin/assets/10217/2b309b0c-ac4a-42ff-9257-03cb75c057ee">
(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="Schermafbeelding 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.
(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`).
(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
...
(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
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1277659822)
Yea, more elegant. Pushed changing it
💬 MarcoFalke commented on pull request "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC":
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655832208)
> Existing callbacks using the node's time to operate could be affected and perform differently
No, they shouldn't be affected by this change. The scheduler thread is already free to behave at any time as-if `syncwithvalidationinterfacequeue` was just called.
This will only reduce intermittent test failure rates where the test assumed that the scheduler thread behaved as-if `syncwithvalidationinterfacequeue` was called and that it *must* happen within a specific time. As explained in the d
...
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655832208)
> Existing callbacks using the node's time to operate could be affected and perform differently
No, they shouldn't be affected by this change. The scheduler thread is already free to behave at any time as-if `syncwithvalidationinterfacequeue` was just called.
This will only reduce intermittent test failure rates where the test assumed that the scheduler thread behaved as-if `syncwithvalidationinterfacequeue` was called and that it *must* happen within a specific time. As explained in the d
...
🚀 fanquake merged a pull request: "ci: Keep system env vars as-is (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28138)
(https://github.com/bitcoin/bitcoin/pull/28138)
👍 furszy approved a pull request: "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC"
(https://github.com/bitcoin/bitcoin/pull/28118#pullrequestreview-1552328188)
Code review ACK fabef121. Convinced by checking all current tests usages.
Thanks Marko.
(https://github.com/bitcoin/bitcoin/pull/28118#pullrequestreview-1552328188)
Code review ACK fabef121. Convinced by checking all current tests usages.
Thanks Marko.
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655892517)
Imo these tests are buggy and are correctly failing here.
> test/logging_tests.cpp:86:77: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
> LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s", "bar1\n");
If we don't enforce that the newline is in the format-string, it's really nasty to enforce that it's "somewhere in one of the arguments", as that could even be added to a string at runtime.
As f
...
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655892517)
Imo these tests are buggy and are correctly failing here.
> test/logging_tests.cpp:86:77: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
> LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s", "bar1\n");
If we don't enforce that the newline is in the format-string, it's really nasty to enforce that it's "somewhere in one of the arguments", as that could even be added to a string at runtime.
As f
...
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470)
nit: It may be good to move this (and `export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu"`) to the only test that needs it. Otherwise it seems odd to export it to all test envs, even if they don't need it?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470)
nit: It may be good to move this (and `export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu"`) to the only test that needs it. Otherwise it seems odd to export it to all test envs, even if they don't need it?
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277721719)
Yea. Refactored to remove `DIR_CORE_TIDY` entirely.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277721719)
Yea. Refactored to remove `DIR_CORE_TIDY` entirely.
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655915934)
Dropped `DIR_CORE_TIDY` and just used `BASE_SCRATCH_DIR` directly.
Added `example_logprintf.cpp` to the lint-format-strings.py exclusion list.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655915934)
Dropped `DIR_CORE_TIDY` and just used `BASE_SCRATCH_DIR` directly.
Added `example_logprintf.cpp` to the lint-format-strings.py exclusion list.