💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2512820195)
... https://cirrus-ci.com/task/5540900196057088?logs=ci#L4684
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2512820195)
... https://cirrus-ci.com/task/5540900196057088?logs=ci#L4684
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866653074)
I don't think we care that much about the exact errors.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866653074)
I don't think we care that much about the exact errors.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659349)
Ok, changed to `LogInfo`.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659349)
Ok, changed to `LogInfo`.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866651913)
I have kept one (in `p2p_segwit.py`), and added a comment about it.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866651913)
I have kept one (in `p2p_segwit.py`), and added a comment about it.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659053)
I have changed it to using `(Script validation error in block: %s", state.ToString())` in both locations, which is close to what the final commit uses too. It feels wrong not to include the detailed information computed in `CScriptCheck::operator()` in this commit already.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659053)
I have changed it to using `(Script validation error in block: %s", state.ToString())` in both locations, which is close to what the final commit uses too. It feels wrong not to include the detailed information computed in `CScriptCheck::operator()` in this commit already.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866660122)
I have changed it to `mandatory_result`, as it may not exactly match the consensus rules.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866660122)
I have changed it to `mandatory_result`, as it may not exactly match the consensus rules.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659604)
Done, more detailed information won't hurt.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866659604)
Done, more detailed information won't hurt.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866656706)
Done, using `LogInfo` for the new code now.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866656706)
Done, using `LogInfo` for the new code now.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866655340)
It would work here, because the unit test only starts a single checker thread, but in general, checkqueue cannot guarantee that what it returns is the *first* error because of thread synchronization. So I'd rather not test for behavior we cannot in general guarantee.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866655340)
It would work here, because the unit test only starts a single checker thread, but in general, checkqueue cannot guarantee that what it returns is the *first* error because of thread synchronization. So I'd rather not test for behavior we cannot in general guarantee.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866653394)
Good idea, added.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866653394)
Good idea, added.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866656047)
Done.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866656047)
Done.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2513004004)
Rebased, and addressed most comments.
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2513004004)
Rebased, and addressed most comments.
📝 brunoerg opened a pull request: "test: fix `test_invalid_tx_in_compactblock` in `p2p_compactblocks`"
(https://github.com/bitcoin/bitcoin/pull/31406)
The `test_invalid_tx_in_compactblock` function tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions. After sending the block with invalid transactions, this test checks 2 things: the tip in the receiver node did not advance and the sender does not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have the necessary transactions to reconstruct the valid and
...
(https://github.com/bitcoin/bitcoin/pull/31406)
The `test_invalid_tx_in_compactblock` function tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions. After sending the block with invalid transactions, this test checks 2 things: the tip in the receiver node did not advance and the sender does not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have the necessary transactions to reconstruct the valid and
...
🤔 furszy reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474045739)
Code review ACK 7b267c034fdc2
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474045739)
Code review ACK 7b267c034fdc2
👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2473436614)
Code review ACK c972e691d937f49d7d3f44d2b19b4035c6e67aaf. Thanks for breaking the commits up make making things easy to understand.
Changes since last review were simplifying the -norpccookiefile and -noconf fixes by breaking them up and changing some implementation details. There are also new combine_logs and feature_config args test improvements.
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2473436614)
Code review ACK c972e691d937f49d7d3f44d2b19b4035c6e67aaf. Thanks for breaking the commits up make making things easy to understand.
Changes since last review were simplifying the -norpccookiefile and -noconf fixes by breaking them up and changing some implementation details. There are also new combine_logs and feature_config args test improvements.
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866689589)
In commit "args: Catch directories in place of config files" (01b174493ec210c1c627083dfa9e9b86035c7eef)
If macos is setting the eof bit when using ifstream to read from a directory paths as described https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947, treating directories like empty files, maybe we really do need these is_directory calls, since there might be no other way on macos to distinguish directories from empty files, though I suspect it probably is indicating some e
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866689589)
In commit "args: Catch directories in place of config files" (01b174493ec210c1c627083dfa9e9b86035c7eef)
If macos is setting the eof bit when using ifstream to read from a directory paths as described https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947, treating directories like empty files, maybe we really do need these is_directory calls, since there might be no other way on macos to distinguish directories from empty files, though I suspect it probably is indicating some e
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866653856)
In commit "args: Properly support -noconf" (2ac863bdf974d7b4b4e8314876d02270400b6240)
I'm still not sure why this is a useful warning. There would seem to be little reason to specify `-noconf` unless you are trying to ignore a config file, so why should you be warned if a file you were trying to ignore exists? If anything, you might expect a warning in the opposite case, when a file you specified an option to ignore does *not* exist. Both warnings seem excessive to me and and I think the app
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866653856)
In commit "args: Properly support -noconf" (2ac863bdf974d7b4b4e8314876d02270400b6240)
I'm still not sure why this is a useful warning. There would seem to be little reason to specify `-noconf` unless you are trying to ignore a config file, so why should you be warned if a file you were trying to ignore exists? If anything, you might expect a warning in the opposite case, when a file you specified an option to ignore does *not* exist. Both warnings seem excessive to me and and I think the app
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866191667
> optional of optional sounds like a `std::variant` - but I'm fine with doing these in a separate PR, of course
Yes could use something like `std::variant<Disabled, not_empty<fs::path>>` if we want a separate type to represent the separate condition. ([`Disabled`](https://github.com/ryanofsky/bitcoin/blob/4e33ed4eb054e230436b68c681d978d0e7bea0a1/src/common/setting.h#L17-L18) could be the type from #31260)). `not_empty
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866191667
> optional of optional sounds like a `std::variant` - but I'm fine with doing these in a separate PR, of course
Yes could use something like `std::variant<Disabled, not_empty<fs::path>>` if we want a separate type to represent the separate condition. ([`Disabled`](https://github.com/ryanofsky/bitcoin/blob/4e33ed4eb054e230436b68c681d978d0e7bea0a1/src/common/setting.h#L17-L18) could be the type from #31260)). `not_empty
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866384411)
In commit "test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning" (eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310
> My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
Don't disagree this commit could be a separate PR. A separate PR might attract feedback from reviewers more familiar with how the test framework is designed to be us
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866384411)
In commit "test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning" (eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310
> My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
Don't disagree this commit could be a separate PR. A separate PR might attract feedback from reviewers more familiar with how the test framework is designed to be us
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866307162)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863342759
> nit Q: I'm not sure I understand the logging quoting standards here, when do we use `\"` and when `'` and when just `=%s`?
I think these are just quoted to convey that 'owner' 'group' and 'all' are literal strings, not placeholders for permission values.
It would be nice if there was a standard way paths and arguments were quoted in log messages, but I don't think there is a standard right now.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866307162)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863342759
> nit Q: I'm not sure I understand the logging quoting standards here, when do we use `\"` and when `'` and when just `=%s`?
I think these are just quoted to convey that 'owner' 'group' and 'all' are literal strings, not placeholders for permission values.
It would be nice if there was a standard way paths and arguments were quoted in log messages, but I don't think there is a standard right now.