💬 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?

💬 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.

💬 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
...

💬 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();
```

💬 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.

💬 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.

👍 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

💬 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.

💬 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

🤔 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.

💬 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.

💬 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
...

🤔 hodlinator reviewed a pull request: "log: always print initial signature verification state"
(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)?

💬 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;
```

💬 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.
-
...

💬 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
...

💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351044620)
Again, `AssumeValid::SKIPPED` meaning script checking is off seems like some weird double-negation to me. Below naming would make more sense to me.
```suggestion
enum class ScriptCheck : uint8_t
{
DISABLED_ASSUME_VALID, //!< skip script verification
ENABLED_NO_ASSUME_VALID, //!< always verify scripts
ENABLED_HASH_NOT_IN_HEADERS, //!< assumevalid hash not found in m_block_index
ENABLED_NOT_UNDER_ASSUMEVALID, //!< pindex is not an ancestor of the assumevalid
...

📝 BrandonOdiwuor opened a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401)
Follow up to https://github.com/bitcoin/bitcoin/pull/32970


https://github.com/bitcoin/bitcoin/pull/32970#r2213730157

> Does `cmake -S ...` still need to be wrapped in `bash -c "..."`?

https://github.com/bitcoin/bitcoin/pull/32970#r2213741192
> It is not trivial to replace. Maybe the `eval` hack from below can be used:
>
> ```shell
> # parses TEST_RUNNER_EXTRA as an array which allows for multiple arguments such as TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6"'
>
> eval
...

🤔 hebasto reviewed a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3229000702)
Concept ACK on expanding ShellCheck lint coverage.