π€ murchandamus reviewed a pull request: "test: add assertions to SRD max weight test"
(https://github.com/bitcoin/bitcoin/pull/33058#pullrequestreview-3080840430)
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
> This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
No, a good test checks that the tested function produces the expected outcomes without relying on knowledge of implementation details.
In this case, the relevant feature
...
(https://github.com/bitcoin/bitcoin/pull/33058#pullrequestreview-3080840430)
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
> This test case has a number of invaluable utxos and some valuable ones. Would it not be better to show that after the min-heap is added, that the implementation has a bias towards selecting valuable utxos? I think these types of implementation details would be valuable to test.
No, a good test checks that the tested function produces the expected outcomes without relying on knowledge of implementation details.
In this case, the relevant feature
...
π¬ Sammie05 commented on pull request "doc: Fix 'getdescriptoractivity' RPCHelpMan":
(https://github.com/bitcoin/bitcoin/pull/33119#issuecomment-3146165616)
Thanks for the fix, As mentioned above, adding a test for this in rpc_getdescriptoractivity.py would make the PR even more robust.
ACK from me for this doc update
(https://github.com/bitcoin/bitcoin/pull/33119#issuecomment-3146165616)
Thanks for the fix, As mentioned above, adding a test for this in rpc_getdescriptoractivity.py would make the PR even more robust.
ACK from me for this doc update
π€ pablomartin4btc reviewed a pull request: "qt: clear command history when clearing the console"
(https://github.com/bitcoin-core/gui/pull/882#pullrequestreview-3080917778)
I don't think this change is practical. Clearing the console should remove the visible output, but it shouldn't also clear the command history. In most terminal environments (e.g., bash, zsh, cmd, PowerShell, etc.), using clear or Ctrl+L clears the screen while preserving command history, which remains accessible via the arrow keys. Users often rely on this history to repeat or modify previous commands, and removing it could unintentionally disrupt that workflow. I suggest keeping the command hi
...
(https://github.com/bitcoin-core/gui/pull/882#pullrequestreview-3080917778)
I don't think this change is practical. Clearing the console should remove the visible output, but it shouldn't also clear the command history. In most terminal environments (e.g., bash, zsh, cmd, PowerShell, etc.), using clear or Ctrl+L clears the screen while preserving command history, which remains accessible via the arrow keys. Users often rely on this history to repeat or modify previous commands, and removing it could unintentionally disrupt that workflow. I suggest keeping the command hi
...
π€ BrandonOdiwuor reviewed a pull request: "cmake: Switch to generated `ts_files.cmake` file"
(https://github.com/bitcoin/bitcoin/pull/33115#pullrequestreview-3080938233)
Code Review ACK f2bfe852236f45b9ea81d6b3f06c6a0097be3ddd
Environment: macOS 15.5, Qt 6.9.0
Build & GUI Test: Built successfully; GUI tested with generated ts_files.cmake via updated update_translations.py. No errors.
Translation Test: Kiswahili (sw) loaded correctly in GUI. Screenshot:
<img width="1098" height="559" alt="Screenshot 2025-08-02 at 09 02 04" src="https://github.com/user-attachments/assets/265a5447-be34-4eeb-94ca-2e9d37ac1cb3" />
Also tested the updated `update_transalt
...
(https://github.com/bitcoin/bitcoin/pull/33115#pullrequestreview-3080938233)
Code Review ACK f2bfe852236f45b9ea81d6b3f06c6a0097be3ddd
Environment: macOS 15.5, Qt 6.9.0
Build & GUI Test: Built successfully; GUI tested with generated ts_files.cmake via updated update_translations.py. No errors.
Translation Test: Kiswahili (sw) loaded correctly in GUI. Screenshot:
<img width="1098" height="559" alt="Screenshot 2025-08-02 at 09 02 04" src="https://github.com/user-attachments/assets/265a5447-be34-4eeb-94ca-2e9d37ac1cb3" />
Also tested the updated `update_transalt
...
π¬ maflcko commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2249160955)
There are still some spacing issues here. See also the report https://github.com/maflcko/b-c-gui-translations-review/blob/ab6dc43a4c543c072d916a268a0bc34562ce3f81/README.md
cc https://github.com/maflcko/DrahtBot/issues/49
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2249160955)
There are still some spacing issues here. See also the report https://github.com/maflcko/b-c-gui-translations-review/blob/ab6dc43a4c543c072d916a268a0bc34562ce3f81/README.md
cc https://github.com/maflcko/DrahtBot/issues/49
π¬ waketraindev commented on pull request "qt: clear command history when clearing the console":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3146305007)
> I don't think this change is practical. Clearing the console should remove the visible output, but it shouldn't also clear the command history. In most terminal environments (e.g., bash, zsh, cmd, PowerShell, etc.), using clear or Ctrl+L clears the screen while preserving command history, which remains accessible via the arrow keys. Users often rely on this history to repeat or modify previous commands, and removing it could unintentionally disrupt that workflow. I suggest keeping the command
...
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3146305007)
> I don't think this change is practical. Clearing the console should remove the visible output, but it shouldn't also clear the command history. In most terminal environments (e.g., bash, zsh, cmd, PowerShell, etc.), using clear or Ctrl+L clears the screen while preserving command history, which remains accessible via the arrow keys. Users often rely on this history to repeat or modify previous commands, and removing it could unintentionally disrupt that workflow. I suggest keeping the command
...
π¬ waketraindev commented on pull request "qt: clear command history when clearing the console":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3146310289)
If there is concern about preserving history by default, a compromise could be to clear command history only when holding Shift while clicking the "Clear" button, similar to how modifier keys are used in other GUIs for alternate actions. But personally, my expectation as a user is that "Clear" fully resets the console, including input history.
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3146310289)
If there is concern about preserving history by default, a compromise could be to clear command history only when holding Shift while clicking the "Clear" button, similar to how modifier keys are used in other GUIs for alternate actions. But personally, my expectation as a user is that "Clear" fully resets the console, including input history.
π¬ jesterhodl commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2249174462)
Thanks, I'll review those
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2249174462)
Thanks, I'll review those
π chareice opened a pull request: "wallet: Improve wallet creation error message with usage hint"
(https://github.com/bitcoin/bitcoin/pull/33124)
Improve wallet creation error message with usage hint
(https://github.com/bitcoin/bitcoin/pull/33124)
Improve wallet creation error message with usage hint
π¬ maflcko commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2249189479)
this has to be guarded, see e.g. the `logthreadnames` below
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2249189479)
this has to be guarded, see e.g. the `logthreadnames` below
π¬ maflcko commented on pull request "wallet: remove use of GetWalletDir and weakly_canonical when creating backup_prefix":
(https://github.com/bitcoin/bitcoin/pull/33122#discussion_r2249190191)
walletname -> wallet name [compound term should be two words for clarity]
if contains slashes -> if it contains slashes [missing pronoun βitβ]
(https://github.com/bitcoin/bitcoin/pull/33122#discussion_r2249190191)
walletname -> wallet name [compound term should be two words for clarity]
if contains slashes -> if it contains slashes [missing pronoun βitβ]
π¬ fanquake commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3146425091)
MSAN failure (https://cirrus-ci.com/task/6435705422872576?logs=ci#L1863) is #33114.
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3146425091)
MSAN failure (https://cirrus-ci.com/task/6435705422872576?logs=ci#L1863) is #33114.
π fanquake opened a pull request: "ci: Use mlc `v1` and ignore `depends/work`"
(https://github.com/bitcoin/bitcoin/pull/33125)
Otherwise you might see failures like this, if there are build artifacts in your depends dir:
```bash
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20031030.htm (39, 55) => /Member/#confidential - Target not found.
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20020521.htm (39, 55) => /Me
...
(https://github.com/bitcoin/bitcoin/pull/33125)
Otherwise you might see failures like this, if there are build artifacts in your depends dir:
```bash
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20031030.htm (39, 55) => /Member/#confidential - Target not found.
[Err ] ./depends/work/build/x86_64-w64-mingw32/qt/6.7.3-64961f1d77c/qtbase/tests/auto/corelib/serialization/qxmlstream/XML-Test-Suite/xmlconf/xmlconf-20020521.htm (39, 55) => /Me
...
π¬ yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3146449310)
> A test being based on knowing that SRD uses the min-heap approach internally would be 1) more complicated, 2) harder to read, 3) more error prone, and 4) a layer violation.
I'm not advocating to test that SRD uses a min-heap. However, the min-heap was added https://github.com/bitcoin/bitcoin/pull/26720 because:
> As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the expectation here is to receive a selection result that only contain the big UTXO. Which is not hap
...
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3146449310)
> A test being based on knowing that SRD uses the min-heap approach internally would be 1) more complicated, 2) harder to read, 3) more error prone, and 4) a layer violation.
I'm not advocating to test that SRD uses a min-heap. However, the min-heap was added https://github.com/bitcoin/bitcoin/pull/26720 because:
> As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the expectation here is to receive a selection result that only contain the big UTXO. Which is not hap
...
π Ataraxia009 opened a pull request: "Allowing multi client support in guix-build"
(https://github.com/bitcoin/bitcoin/pull/33126)
Using `CLIENT_NAME` variable in guix build files to better support forked clients
(https://github.com/bitcoin/bitcoin/pull/33126)
Using `CLIENT_NAME` variable in guix build files to better support forked clients
π€ furszy reviewed a pull request: "test: add assertions to SRD max weight test"
(https://github.com/bitcoin/bitcoin/pull/33058#pullrequestreview-3081012887)
utACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
(https://github.com/bitcoin/bitcoin/pull/33058#pullrequestreview-3081012887)
utACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
π Ataraxia009 opened a pull request: "Adding alert for failure to prevent dead-end user crash"
(https://github.com/bitcoin/bitcoin/pull/33127)
In some cases after running the client for a bit, the chainstate seems to have blocks with a lower validity level than `VALID_SCRIPTS`. This causes a crash on launch in the `PruneBlockIndexCandidates` function, where the `setBlockIndexCandidates` is empty after filtering by m_chain.
Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
(https://github.com/bitcoin/bitcoin/pull/33127)
In some cases after running the client for a bit, the chainstate seems to have blocks with a lower validity level than `VALID_SCRIPTS`. This causes a crash on launch in the `PruneBlockIndexCandidates` function, where the `setBlockIndexCandidates` is empty after filtering by m_chain.
Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
π€ furszy reviewed a pull request: "Adding alert for failure to prevent dead-end user crash"
(https://github.com/bitcoin/bitcoin/pull/33127#pullrequestreview-3081017861)
> Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
Could you share steps to reproduce it?
(https://github.com/bitcoin/bitcoin/pull/33127#pullrequestreview-3081017861)
> Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
Could you share steps to reproduce it?
π¬ Ataraxia009 commented on pull request "Adding alert for failure to prevent dead-end user crash":
(https://github.com/bitcoin/bitcoin/pull/33127#issuecomment-3146474055)
> > Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
>
> Could you share steps to reproduce it?
Couldn't really reproduce it myself, just a had a data dir in a bad state
Which, if this assert is true, should imply we are in a bad data dir state where the chainstate is not included in the setBlockIndexCandidates and all setBlockIndexCandidates are less work.
(https://github.com/bitcoin/bitcoin/pull/33127#issuecomment-3146474055)
> > Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
>
> Could you share steps to reproduce it?
Couldn't really reproduce it myself, just a had a data dir in a bad state
Which, if this assert is true, should imply we are in a bad data dir state where the chainstate is not included in the setBlockIndexCandidates and all setBlockIndexCandidates are less work.
π€ hodlinator reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3081014156)
Reviewed 1523e8d6c981efff1394bd1669fbbbea5ff7ac97
Still had some late suggestions which will hopefully get this in better shape for other reviewers too.
Might be worth pinging some participants from #27591 at this point to see what they think.
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3081014156)
Reviewed 1523e8d6c981efff1394bd1669fbbbea5ff7ac97
Still had some late suggestions which will hopefully get this in better shape for other reviewers too.
Might be worth pinging some participants from #27591 at this point to see what they think.