💬 RandyMcMillan commented on pull request "[wip] wallet: Add separate balance info for non-mempool wallet txs":
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3511602426)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3511602426)
Concept ACK
💬 fanquake commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510562617)
Rebased #33775 on this, and dropped the workarounds back out.
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510562617)
Rebased #33775 on this, and dropped the workarounds back out.
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2510587958)
Now that #33785 has been merged we hopefully can use `const& ... Assert(` in this PR as long as it's rebased.
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2510587958)
Now that #33785 has been merged we hopefully can use `const& ... Assert(` in this PR as long as it's rebased.
📝 waketraindev converted_to_draft a pull request: "Prevent re-execution of sensitive commands from console history"
(https://github.com/bitcoin-core/gui/pull/909)
Sensitive RPC commands such as `walletpassphrase` or `createwallet`
may appear in the console history with their arguments redacted. Previously,
these entries could still be re-executed if recalled, potentially causing
unintended actions.
This change prefixes sensitive history entries with a leading character(`!`),
marking them as non-executable when called. The console blocks their
execution and informs the user that the command was blocked.
The help text in `help-console` has been upd
...
(https://github.com/bitcoin-core/gui/pull/909)
Sensitive RPC commands such as `walletpassphrase` or `createwallet`
may appear in the console history with their arguments redacted. Previously,
these entries could still be re-executed if recalled, potentially causing
unintended actions.
This change prefixes sensitive history entries with a leading character(`!`),
marking them as non-executable when called. The console blocks their
execution and informs the user that the command was blocked.
The help text in `help-console` has been upd
...
💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3511766342)
> I guess we are now just blocked on Qt / GUI tooling...
From https://github.com/bitcoin/bitcoin/pull/32648#issuecomment-3027824977:
> this should be fixed in Qts tooling.
Even if it’s been fixed upstream, I don’t expect that change to be backported to all Qt versions down to 6.2. Therefore, we still need to suppress warnings in generated files. For example, as follows:
```diff
--- a/src/qt/CMakeLists.txt
+++ b/src/qt/CMakeLists.txt
@@ -50,11 +50,20 @@ endfunction()
set(CMAKE_AUT
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3511766342)
> I guess we are now just blocked on Qt / GUI tooling...
From https://github.com/bitcoin/bitcoin/pull/32648#issuecomment-3027824977:
> this should be fixed in Qts tooling.
Even if it’s been fixed upstream, I don’t expect that change to be backported to all Qt versions down to 6.2. Therefore, we still need to suppress warnings in generated files. For example, as follows:
```diff
--- a/src/qt/CMakeLists.txt
+++ b/src/qt/CMakeLists.txt
@@ -50,11 +50,20 @@ endfunction()
set(CMAKE_AUT
...
💬 maflcko commented on pull request "refactor: Add missing include in bitcoinkernel_wrapper.h":
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511768897)
> [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511768897)
> [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
💬 fanquake commented on pull request "Update `minisketch` subtree":
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3511834935)
ACK c235aa468b0dcc67b49340dbe9b675c513cec7bf
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3511834935)
ACK c235aa468b0dcc67b49340dbe9b675c513cec7bf
🤔 furszy reviewed a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-3443301033)
ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
No need to tackle the nano nits I left.
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-3443301033)
ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
No need to tackle the nano nits I left.
💬 furszy commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510608464)
In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
nano nit: I'm not sure how helpful this comment is.
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510608464)
In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
nano nit: I'm not sure how helpful this comment is.
💬 furszy commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510648818)
In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
nano nit: you are already mentioning this above the for-loop line.
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510648818)
In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
nano nit: you are already mentioning this above the for-loop line.
💬 furszy commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510613538)
In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
Pedantic ultra-nano nit:
We finish comments with a dot only if they span more than one line.
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2510613538)
In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a:
Pedantic ultra-nano nit:
We finish comments with a dot only if they span more than one line.
💬 hebasto commented on pull request "refactor: Add missing include in bitcoinkernel_wrapper.h":
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511843037)
> > [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
>
> I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
It's here: https://github.com/hebasto/bitcoin/actions/runs/19194191546/job/54872770781:
```
<snip>
2025-11-08T14:21:35.1873049Z (/home/runner/work/_temp/src/kernel/bitcoinkernel_wrapper.h
...
(https://github.com/bitcoin/bitcoin/pull/33825#issuecomment-3511843037)
> > [Here](https://github.com/hebasto/bitcoin/commit/11bc5cca61a9edad38d070bd5046355fcf58c7ee#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2) is IWYU's diff based on #33810:
>
> I can't find that in the CI output. I don't think iwyu runs on stand-alone headers without a cpp file?
It's here: https://github.com/hebasto/bitcoin/actions/runs/19194191546/job/54872770781:
```
<snip>
2025-11-08T14:21:35.1873049Z (/home/runner/work/_temp/src/kernel/bitcoinkernel_wrapper.h
...
🚀 fanquake merged a pull request: "Update `minisketch` subtree"
(https://github.com/bitcoin/bitcoin/pull/32856)
(https://github.com/bitcoin/bitcoin/pull/32856)
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2510713428)
Ran this patch against master with -reindex (which puts lots of entries in `vBlocks`) and noticed no slowdown.
cc @l0rinc, do you have any opinions about this? We need this for deterministic fuzzing, we can also wrap this in a fuzz-specific macro.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2510713428)
Ran this patch against master with -reindex (which puts lots of entries in `vBlocks`) and noticed no slowdown.
cc @l0rinc, do you have any opinions about this? We need this for deterministic fuzzing, we can also wrap this in a fuzz-specific macro.
⚠️ charletonjoan1 opened an issue: "REPORT AND RECOVER MONEY BACK FROM A FRAUDULENT CHARITY/DONATION SCAM.HIRE META TECH RECOVERY PRO"
(https://github.com/bitcoin/bitcoin/issues/33839)
A charity organization popped up on my screen as an ad. They had a professional-looking website, emotional stories, and testimonials from supposed donors. They had pictures of their representatives with A-list actors, celebrities, and philanthropists as donors; unfortunately, they were edited. I read about their services and outreach, and I was motivated to give a token as a widow with no children or close family relatives. They claimed the funds sourced were being used to support families in n
...
(https://github.com/bitcoin/bitcoin/issues/33839)
A charity organization popped up on my screen as an ad. They had a professional-looking website, emotional stories, and testimonials from supposed donors. They had pictures of their representatives with A-list actors, celebrities, and philanthropists as donors; unfortunately, they were edited. I read about their services and outreach, and I was motivated to give a token as a widow with no children or close family relatives. They claimed the funds sourced were being used to support families in n
...
💬 maflcko commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510727269)
> k, I had the impression this was the only one 👍
Oh, I think I misunderstood the previous comments. Clearly a test-only workaround to a single file is better than a ci-wide workaround to all files.
The test code is "wrong" anyway. Using a pointer here makes little sense, when a reference should be used.
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510727269)
> k, I had the impression this was the only one 👍
Oh, I think I misunderstood the previous comments. Clearly a test-only workaround to a single file is better than a ci-wide workaround to all files.
The test code is "wrong" anyway. Using a pointer here makes little sense, when a reference should be used.
👍 TheCharlatan approved a pull request: "kernel: trim Chain interface"
(https://github.com/bitcoin/bitcoin/pull/33820#pullrequestreview-3443473061)
ACK 66978a1a95379a2fe5d41032682dedfaddc99db9
(https://github.com/bitcoin/bitcoin/pull/33820#pullrequestreview-3443473061)
ACK 66978a1a95379a2fe5d41032682dedfaddc99db9
💬 fanquake commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3511924176)
> Even if it’s been fixed upstream,
It's not clear to me if it has been fixed or not, can you link to the relevant change / issue? I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3511924176)
> Even if it’s been fixed upstream,
It's not clear to me if it has been fixed or not, can you link to the relevant change / issue? I can cherry-pick the change above in, if you create a commit, and add a comment inline, for when the change can be dropped.
📝 maflcko opened a pull request: " test: [refactor] Use reference over ptr to chainman "
(https://github.com/bitcoin/bitcoin/pull/33840)
Just some minor test-only refactor commits to fix GCC false positive warnings, along with making the test code easier to read and understand:
* First change requested in https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510727269
* Second change requested in commit 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf
Those changes are required in a bunch of pulls touching the CI system, so merging them allows to drop them in all pulls.
(https://github.com/bitcoin/bitcoin/pull/33840)
Just some minor test-only refactor commits to fix GCC false positive warnings, along with making the test code easier to read and understand:
* First change requested in https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2510727269
* Second change requested in commit 3b135a8fc4451c93b3ea50b3f4621e0d19f35daf
Those changes are required in a bunch of pulls touching the CI system, so merging them allows to drop them in all pulls.
📝 hebasto opened a pull request: "cmake: Switch to `minisketch` upstream build system"
(https://github.com/bitcoin/bitcoin/pull/33841)
This PR:
1. Is split out from https://github.com/bitcoin/bitcoin/pull/32856 as [requested](https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3511268570).
2. Switches to `minisketch`'s own build system script.
3. Does not change behaviour, except for printing "minisketch configure summary" in the build configuration output.
Fellow reviewers might also be interested in the discussion that took place in https://github.com/bitcoin/bitcoin/pull/32856.
(https://github.com/bitcoin/bitcoin/pull/33841)
This PR:
1. Is split out from https://github.com/bitcoin/bitcoin/pull/32856 as [requested](https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3511268570).
2. Switches to `minisketch`'s own build system script.
3. Does not change behaviour, except for printing "minisketch configure summary" in the build configuration output.
Fellow reviewers might also be interested in the discussion that took place in https://github.com/bitcoin/bitcoin/pull/32856.