π€ real-or-random reviewed a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3228076799)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3228076799)
Concept ACK
π¬ musaHaruna commented on issue "wallet RPC to double-check the calculated balance":
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-3296357283)
Hi everyone,
Iβve opened PR #[28930](https://github.com/bitcoin/bitcoin/pull/33392) that attempts to resolve the issue.
The PR adds a scan_utxoset flag to getbalance / getbalances that lets the wallet independently compute a balance by scanning the UTXO set. If a discrepancy is found, the RPC response includes both balances plus a suggested action (rescanblockchain or reimporting descriptors with the correct timestamp).
This doesnβt replace rescanning β itβs a diagnostic check so users can de
...
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-3296357283)
Hi everyone,
Iβve opened PR #[28930](https://github.com/bitcoin/bitcoin/pull/33392) that attempts to resolve the issue.
The PR adds a scan_utxoset flag to getbalance / getbalances that lets the wallet independently compute a balance by scanning the UTXO set. If a discrepancy is found, the RPC response includes both balances plus a suggested action (rescanblockchain or reimporting descriptors with the correct timestamp).
This doesnβt replace rescanning β itβs a diagnostic check so users can de
...
π¬ ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3296407130)
Dropped the code to use our own prior templates for compact block reconstruction to keep this patchset simple.
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3296407130)
Dropped the code to use our own prior templates for compact block reconstruction to keep this patchset simple.
π¬ Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2351335279)
That doesn't really say much, because indirect includes will succeed but we try to avoid those. Though maybe not for std stuff?
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2351335279)
That doesn't really say much, because indirect includes will succeed but we try to avoid those. Though maybe not for std stuff?
π¬ yuvicc commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3296474933)
- I have upgraded the version of my nodes from v29.1 -> v30.0rc1, everything looks stable till now.
- Successfully ran the [testing guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide) on Ubuntu 24.04 and macOS 15.2.
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3296474933)
- I have upgraded the version of my nodes from v29.1 -> v30.0rc1, everything looks stable till now.
- Successfully ran the [testing guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide) on Ubuntu 24.04 and macOS 15.2.
π¬ vasild commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2351406219)
Previously `AddWhitelistPermissionFlags()` would have been called for incoming tor connections. It does this:
```cpp
void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
for (const auto& subnet : ranges) {
if (subnet.m_subnet.Match(addr)) {
NetPermissions::AddFlag(flags, subnet.m_flags);
}
}
if (NetPermissions::HasFlag(flags, NetPermissionFlags::Imp
...
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2351406219)
Previously `AddWhitelistPermissionFlags()` would have been called for incoming tor connections. It does this:
```cpp
void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
for (const auto& subnet : ranges) {
if (subnet.m_subnet.Match(addr)) {
NetPermissions::AddFlag(flags, subnet.m_flags);
}
}
if (NetPermissions::HasFlag(flags, NetPermissionFlags::Imp
...
π¬ vasild commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2351365803)
nit, this is just moving code around without modifying it, feel free to ignore; can be written shorter as:
```suggestion
const bool inbound_onion = std::ranges::find(m_onion_binds, addr_bind) != m_onion_binds.end();
```
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2351365803)
nit, this is just moving code around without modifying it, feel free to ignore; can be written shorter as:
```suggestion
const bool inbound_onion = std::ranges::find(m_onion_binds, addr_bind) != m_onion_binds.end();
```
π¬ janb84 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296644767)
Please:
- remove the .claude folder and contents
- change pr description or remove the "How Bitcoin's Script Virtual Machine Works" document
As an additional note, please declare the usage of the AI, if applicable.
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296644767)
Please:
- remove the .claude folder and contents
- change pr description or remove the "How Bitcoin's Script Virtual Machine Works" document
As an additional note, please declare the usage of the AI, if applicable.
π¬ fanquake commented on pull request "test: Prevent disk space warning during node_init_tests":
(https://github.com/bitcoin/bitcoin/pull/33391#issuecomment-3296722478)
Backported to 30.x in #33356.
(https://github.com/bitcoin/bitcoin/pull/33391#issuecomment-3296722478)
Backported to 30.x in #33356.
π TheCharlatan approved a pull request: "multiprocess: Don't require bitcoin -m argument when IPC options are used"
(https://github.com/bitcoin/bitcoin/pull/33229#pullrequestreview-3228511746)
ACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86
(https://github.com/bitcoin/bitcoin/pull/33229#pullrequestreview-3228511746)
ACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86
π¬ optout21 commented on pull request "refactor: Fix typo and correct template parameter inconsistency":
(https://github.com/bitcoin/bitcoin/pull/33394#issuecomment-3296775924)
Concept NACK
Though the changes are reasonable, I don't think a typo fix in a variable name justifies a separate PR.
(https://github.com/bitcoin/bitcoin/pull/33394#issuecomment-3296775924)
Concept NACK
Though the changes are reasonable, I don't think a typo fix in a variable name justifies a separate PR.
π¬ jalateras commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296807458)
@janb84
1. I have removed unrelated files
2. I have squashed and pushed
3. I have added an AI declaration in the PR
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296807458)
@janb84
1. I have removed unrelated files
2. I have squashed and pushed
3. I have added an AI declaration in the PR
π€ optout21 reviewed a pull request: "refactor: Fix typo and correct template parameter inconsistency"
(https://github.com/bitcoin/bitcoin/pull/33394#pullrequestreview-3228547436)
Concept NACK
Though the changes are reasonable, I don't think a typo fix in a variable name justifies a separate PR.
(https://github.com/bitcoin/bitcoin/pull/33394#pullrequestreview-3228547436)
Concept NACK
Though the changes are reasonable, I don't think a typo fix in a variable name justifies a separate PR.
π¬ TheCharlatan commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296948637)
NACK
Besides reading heavily AI-generated, this information does not seem useful, since it does not describe concretely how to actually make use of any of our shipped build artefacts.
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3296948637)
NACK
Besides reading heavily AI-generated, this information does not seem useful, since it does not describe concretely how to actually make use of any of our shipped build artefacts.
π¬ ismaelsadeeq commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3297095317)
Thanks for taking a look.
@ryanofsky wrote
> It does seem like BlockTemplateManager is currently duplicating some logic in WaitAndCreateNewBlock, but I think the idea would be to replace it?
For now I think it wonβt exactly replace it; instead,
```cpp
auto new_tmpl{BlockAssembler{
chainman.ActiveChainstate(),
mempool,
assemble_options}
.CreateNewBlock()};
```
should be replaced with
```cpp
auto interv
...
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3297095317)
Thanks for taking a look.
@ryanofsky wrote
> It does seem like BlockTemplateManager is currently duplicating some logic in WaitAndCreateNewBlock, but I think the idea would be to replace it?
For now I think it wonβt exactly replace it; instead,
```cpp
auto new_tmpl{BlockAssembler{
chainman.ActiveChainstate(),
mempool,
assemble_options}
.CreateNewBlock()};
```
should be replaced with
```cpp
auto interv
...
π€ hodlinator reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3227682434)
Reviewed 9b04e4f96200daf7516d1b8b6b0dfc4c077837e8
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3227682434)
Reviewed 9b04e4f96200daf7516d1b8b6b0dfc4c077837e8
π¬ hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351004321)
In e4077fe0d7ae245e1dd3e89903e818703c0ab130: `fScriptChecks` seems to be the opposite of what I would expect `assume_valid` to mean (no script checking)?
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351004321)
In e4077fe0d7ae245e1dd3e89903e818703c0ab130: `fScriptChecks` seems to be the opposite of what I would expect `assume_valid` to mean (no script checking)?
π¬ hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351556972)
It feels a bit lacking to have `CHECKED_NOT_UNDER_ASSUMEVALID` when the block is based upon the assumevalid block. Would expect something like `CHECKED_EXCEEDS_ASSUMEVALID`. For *validation.cpp*:
```C++
} else if (it->second.nHeight < pindex->nHeight) {
assume_valid = CHECKED_EXCEEDS_ASSUMEVALID;
} else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID;
```
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351556972)
It feels a bit lacking to have `CHECKED_NOT_UNDER_ASSUMEVALID` when the block is based upon the assumevalid block. Would expect something like `CHECKED_EXCEEDS_ASSUMEVALID`. For *validation.cpp*:
```C++
} else if (it->second.nHeight < pindex->nHeight) {
assume_valid = CHECKED_EXCEEDS_ASSUMEVALID;
} else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID;
```
π¬ hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351302165)
Could be more precise in that these strings are on the same log line:
```diff
diff --git a/test/functional/feature_assumevalid.py b/test/functional/feature_assumevalid.py
index 544edfdb99..9ebaddb74f 100755
--- a/test/functional/feature_assumevalid.py
+++ b/test/functional/feature_assumevalid.py
@@ -150,8 +150,9 @@ class AssumeValidTest(BitcoinTestFramework):
p2p0.send_header_for_blocks(self.blocks[2000:])
# Send blocks to node0. Block 102 will be rejected.
-
...
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351302165)
Could be more precise in that these strings are on the same log line:
```diff
diff --git a/test/functional/feature_assumevalid.py b/test/functional/feature_assumevalid.py
index 544edfdb99..9ebaddb74f 100755
--- a/test/functional/feature_assumevalid.py
+++ b/test/functional/feature_assumevalid.py
@@ -150,8 +150,9 @@ class AssumeValidTest(BitcoinTestFramework):
p2p0.send_header_for_blocks(self.blocks[2000:])
# Send blocks to node0. Block 102 will be rejected.
-
...
π¬ hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351694634)
If we get here, we know from the previous check (`if (it->second.GetAncestor(pindex->nHeight) != pindex) assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID`) that `pindex` now *does exist* below the assumevalid header (or *is* the assumevalid block).
For the check against best header chain to fail for `pindex`, it could either be:
1. The best header height somehow being below both `pindex` *and assumevalid*.
2. The best header chain being a totally different chain that doesn't even include the
...
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351694634)
If we get here, we know from the previous check (`if (it->second.GetAncestor(pindex->nHeight) != pindex) assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID`) that `pindex` now *does exist* below the assumevalid header (or *is* the assumevalid block).
For the check against best header chain to fail for `pindex`, it could either be:
1. The best header height somehow being below both `pindex` *and assumevalid*.
2. The best header chain being a totally different chain that doesn't even include the
...