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

🚀 fanquake merged a pull request: "depends: systemtap 5.3"
(https://github.com/bitcoin/bitcoin/pull/33373)

💬 pinheadmz commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#discussion_r2351871877)
right thanks!

💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351892330)
Found a way to test 2):
```python
# nodes[4]
self.start_node(4, extra_args=["-assumevalid=" + block102.hash_hex])
second_chain_tip = int(self.nodes[4].getbestblockhash(), 16)
second_chain_time = self.nodes[4].getblock(self.nodes[4].getbestblockhash())['time'] + 1
second_chain_height = self.nodes[4].getblock(self.nodes[4].getbestblockhash())['height'] + 1

second_chain = []
for _ in range(2100):
block = create_block(sec
...

📝 mstampfer opened a pull request: "contrib: Add zsh completion scripts"
(https://github.com/bitcoin/bitcoin/pull/33402)
Adds zsh completion support using the compdef system for:
- bitcoin-cli with RPC command completion and context-sensitive arguments
- bitcoin-tx with option/command completion and file path handling
- bitcoind with configuration option completion

These complement the existing bash completion scripts and provide
the same functionality for zsh users.

💬 mstampfer commented on pull request "contrib: Add zsh completion scripts":
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3297709907)
This PR adds zsh completion scripts for Bitcoin Core tools, converting the existing bash completions to work with zsh's compdef system. This Improves user experience for zsh users by addressing the need for tab completion support for zsh which doesn't rely on the existing bash completion scripts.

The implementation maintains the same functionality as the bash versions while using zsh-native completion functions.

I'm happy to address any concerns or make requested changes.

🤔 marcofleon reviewed a pull request: "test: Add submitblock test in interface_ipc"
(https://github.com/bitcoin/bitcoin/pull/33380#pullrequestreview-3229240434)
code review ACK 0a26731c4cc182e887ce959cdd301227cdc752d7

✅ fanquake closed a pull request: "util/strencodings: guard SAFE_CHARS index and pre-reserve result"
(https://github.com/bitcoin/bitcoin/pull/33396)

💬 fanquake commented on pull request "util/strencodings: guard SAFE_CHARS index and pre-reserve result":
(https://github.com/bitcoin/bitcoin/pull/33396#issuecomment-3297809297)
Thanks, however we can leave this code as-is.

✅ fanquake closed a pull request: "refactor: Fix typo and correct template parameter inconsistency"
(https://github.com/bitcoin/bitcoin/pull/33394)

💬 fanquake commented on pull request "refactor: Fix typo and correct template parameter inconsistency":
(https://github.com/bitcoin/bitcoin/pull/33394#issuecomment-3297824742)
Thanks, however I think this code can be left as-is for now.

💬 TheCharlatan commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2352051546)
Are the enums really only added for logging? That seems a bit heavy. How about moving this `if` block up and making it a lambda that gets called directly from your `if else` branches? afaict that could remove the need for them, which judged by the comments left here so far, seem to confuse people a bit with their communicated intent.