💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2512564803)
@ryanofsky that one and https://github.com/chaincodelabs/libmultiprocess/issues/122 are the only two I'm aware of at the moment.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2512564803)
@ryanofsky that one and https://github.com/chaincodelabs/libmultiprocess/issues/122 are the only two I'm aware of at the moment.
📝 mzumsande opened a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405)
Historically, some fields in validation were set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there were known situations in which these fields were not set when they should've been, or set to wrong values. This was tolerated because the fields were not used for anything consensus-critical and triggering these situations inv
...
(https://github.com/bitcoin/bitcoin/pull/31405)
Historically, some fields in validation were set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there were known situations in which these fields were not set when they should've been, or set to wrong values. This was tolerated because the fields were not used for anything consensus-critical and triggering these situations inv
...
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1866522517)
IIRC chmod does not work on Windows, so a workaround may be needed for the test failure.
Not sure about the `wallet_migration.py` failure. Another workaround may be needed there.
Also, the `wallet_migration` test won't be running after https://github.com/bitcoin/bitcoin/pull/31248, so the previous releases tests should probably be run as well here.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1866522517)
IIRC chmod does not work on Windows, so a workaround may be needed for the test failure.
Not sure about the `wallet_migration.py` failure. Another workaround may be needed there.
Also, the `wallet_migration` test won't be running after https://github.com/bitcoin/bitcoin/pull/31248, so the previous releases tests should probably be run as well here.
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1866523690)
(Functional tests could be handled in a follow-up, if you want)
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1866523690)
(Functional tests could be handled in a follow-up, if you want)
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866581222)
Exactly, the existing code is inadvertently testing the wrong thing (it's invalid, but not for the reason the test seems to aim for).
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1866581222)
Exactly, the existing code is inadvertently testing the wrong thing (it's invalid, but not for the reason the test seems to aim for).
💬 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.