💬 RandyMcMillan commented on pull request "rpcconsole: display signet challenge":
(https://github.com/bitcoin-core/gui/pull/896#issuecomment-3339414175)
use HexStr() per [pablomartin4btc](https://github.com/pablomartin4btc) suggestion
(https://github.com/bitcoin-core/gui/pull/896#issuecomment-3339414175)
use HexStr() per [pablomartin4btc](https://github.com/pablomartin4btc) suggestion
📝 purpleKarrot opened a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483)
Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.
This should be upstreamed to CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/26920
Left to do: The `test_bitcoin-qt` executable should be fixed to support the `-function` option documented here: https://doc.qt.io/qt-6/qtest-overview.html#testing-options so that its test cases can be discovered as well.
(https://github.com/bitcoin/bitcoin/pull/33483)
Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.
This should be upstreamed to CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/26920
Left to do: The `test_bitcoin-qt` executable should be fixed to support the `-function` option documented here: https://doc.qt.io/qt-6/qtest-overview.html#testing-options so that its test cases can be discovered as well.
💬 purpleKarrot commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3339779236)
Please see https://github.com/bitcoin/bitcoin/pull/33483 for dynamic test discovery.
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3339779236)
Please see https://github.com/bitcoin/bitcoin/pull/33483 for dynamic test discovery.
📝 theStack opened a pull request: "doc: rpc: fix case typo in `finalizepsbt` help (final_scriptwitness)"
(https://github.com/bitcoin/bitcoin/pull/33484)
The lower-case spelling matches the `decodepsbt` result field:
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L871
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L1253
(https://github.com/bitcoin/bitcoin/pull/33484)
The lower-case spelling matches the `decodepsbt` result field:
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L871
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L1253
👍 l0rinc approved a pull request: "doc: rpc: fix case typo in `finalizepsbt` help (final_scriptwitness)"
(https://github.com/bitcoin/bitcoin/pull/33484#pullrequestreview-3273312375)
ACK ff05bebcc4262966b117082a67dc4c63a3f67d2d
The doc seems to be the only mention of `final_scriptWitness`, every other one is indeed `final_scriptwitness` - seems it was added incorrectly in https://github.com/bitcoin/bitcoin/commit/c27fe419efb3b6588c400d764122ffb33375e028#diff-a58e7bb9d9a8a0287c0b7281d99da4e79b6f8c2a5780c24c6d76c14212c48640R1624
(https://github.com/bitcoin/bitcoin/pull/33484#pullrequestreview-3273312375)
ACK ff05bebcc4262966b117082a67dc4c63a3f67d2d
The doc seems to be the only mention of `final_scriptWitness`, every other one is indeed `final_scriptwitness` - seems it was added incorrectly in https://github.com/bitcoin/bitcoin/commit/c27fe419efb3b6588c400d764122ffb33375e028#diff-a58e7bb9d9a8a0287c0b7281d99da4e79b6f8c2a5780c24c6d76c14212c48640R1624
💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383126356)
Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place, the current solution feels like a work-around to me - which is still better than what we had, but it's also arguably more complicated and it doesn't seem to fix the underlying problem of why reindex is missing headers in the first place. And are we sure it's a reindex-only bug or can it happen for reindex-chainstate or IBD as well? I haven't dug too deep into this, but I wan
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383126356)
Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place, the current solution feels like a work-around to me - which is still better than what we had, but it's also arguably more complicated and it doesn't seem to fix the underlying problem of why reindex is missing headers in the first place. And are we sure it's a reindex-only bug or can it happen for reindex-chainstate or IBD as well? I haven't dug too deep into this, but I wan
...
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383164921)
The test failure with `send` was because of sighash types, not that `send` doesn't work for musig. It was failing on the test that changes the sighash type.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383164921)
The test failure with `send` was because of sighash types, not that `send` doesn't work for musig. It was failing on the test that changes the sighash type.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383168777)
The sighash is calculated multiple times during signing outside of musig signing too. I will leave refactoring of this to a followup.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383168777)
The sighash is calculated multiple times during signing outside of musig signing too. I will leave refactoring of this to a followup.
💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383171731)
> Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place
I tried to describe this in the linked issue, I think the problem is understood. To rephrase it:
Normal IBD has the order 1) Download entire header chain until we have crossed minchainwork 2) Start downloading blocks
=> assumevalid will be applied because we have a header with minchainwork and the assumevalid block in our index
During reindex, we 1) Delete all ex
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383171731)
> Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place
I tried to describe this in the linked issue, I think the problem is understood. To rephrase it:
Normal IBD has the order 1) Download entire header chain until we have crossed minchainwork 2) Start downloading blocks
=> assumevalid will be applied because we have a header with minchainwork and the assumevalid block in our index
During reindex, we 1) Delete all ex
...
💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3340123470)
Seems nice, but in the great picture there is still room for improvement:
* This doesn't address the issue of running cross builds on the target (https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/.github/workflows/ci.yml#L388)
* The functional tests aren't integrated, so they are run sequentially after the unit tests
I am not going to block this pull, because it is a clear improvement over master, and the other issues should be fixed in separate pulls. Howev
...
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3340123470)
Seems nice, but in the great picture there is still room for improvement:
* This doesn't address the issue of running cross builds on the target (https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/.github/workflows/ci.yml#L388)
* The functional tests aren't integrated, so they are run sequentially after the unit tests
I am not going to block this pull, because it is a clear improvement over master, and the other issues should be fixed in separate pulls. Howev
...
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499341)
Unified.
> Right now, FillSignatureData is only called on new empty SignatureData objects, but not sure if that is an assumption in the code.
That is not an assumption.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499341)
Unified.
> Right now, FillSignatureData is only called on new empty SignatureData objects, but not sure if that is an assumption in the code.
That is not an assumption.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499483)
Done
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499483)
Done
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499646)
Done
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499646)
Done
💬 Fiach-Dubh commented on something "":
(https://github.com/bitcoin/bitcoin/commit/766561b8297b534dac1ed7ccfc57e03fa5a41043#commitcomment-166657363)
we can't give luke-jr the nuclear launch codes!
(https://github.com/bitcoin/bitcoin/commit/766561b8297b534dac1ed7ccfc57e03fa5a41043#commitcomment-166657363)
we can't give luke-jr the nuclear launch codes!
🤔 hodlinator reviewed a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3273948902)
Latest push fixes bug from previous push (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379926157), simplifies code (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379920746) and adds commit to fix docstring (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379958513).
Compared run-times for all functional tests and wasn't able to find one which clearly benefited from this optimization. So I'm open to just replacing `while remaining_expected and remaining_ex
...
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3273948902)
Latest push fixes bug from previous push (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379926157), simplifies code (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379920746) and adds commit to fix docstring (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379958513).
Compared run-times for all functional tests and wasn't able to find one which clearly benefited from this optimization. So I'm open to just replacing `while remaining_expected and remaining_ex
...
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490859)
Regarding nit: I sympathize with the way `busy_wait_for_debug_log()` on master puts the parameter to `_raise_assertion_error()` on it's own line, so I would be tempted to do that for all of them - which makes me realize: if only I broke the message in two lines, I could raise up the first half and still keep close to 80 char width - and we're back with the current approach. Left unchanged in latest push.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490859)
Regarding nit: I sympathize with the way `busy_wait_for_debug_log()` on master puts the parameter to `_raise_assertion_error()` on it's own line, so I would be tempted to do that for all of them - which makes me realize: if only I broke the message in two lines, I could raise up the first half and still keep close to 80 char width - and we're back with the current approach. Left unchanged in latest push.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490580)
Thanks for noticing this issue!
Latest push inserts
```python
remaining_expected = [e for e in remaining_expected if e not in log]
```
before the error reporting for it to be correct, meaning we only need to do the full search on failure.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490580)
Thanks for noticing this issue!
Latest push inserts
```python
remaining_expected = [e for e in remaining_expected if e not in log]
```
before the error reporting for it to be correct, meaning we only need to do the full search on failure.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383497595)
Thanks! Added commit removing this from docstring with reference in commit message to ref commit where it was made incorrect.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383497595)
Thanks! Added commit removing this from docstring with reference in commit message to ref commit where it was made incorrect.
👍 hodlinator approved a pull request: "Modernize use of UTF-8 in Windows code"
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3274017245)
re-ACK 63f3ea2b041db56487bfb92813e32c0bfef6faa1
Confirmed to resolve conflict with #33229.
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3274017245)
re-ACK 63f3ea2b041db56487bfb92813e32c0bfef6faa1
Confirmed to resolve conflict with #33229.
💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3340619538)
If this is backported, release notes https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#ipc-mining-interface could be be simplified to not mention `-m` at all, which would be nice
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3340619538)
If this is backported, release notes https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#ipc-mining-interface could be be simplified to not mention `-m` at all, which would be nice