💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544897954)
Thanks @theStack, Fixed `create_empty_fork` in `mempool_updatefromblock.py` moving from second commit to first.

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536251389)
Placement doesn't matter, but I don't think moving closer to COMMANDS, REGISTER_COMMANDS or CLI_COMMANDS really makes sense -- COMMAND_OPTIONS relates to `argsman.AddCommand()` commands (used by bitcoin-wallet and bitcoin-util), which differ from `COMMANDS` (used by bitcoin-tx), `REGISTER_COMMANDS` (also used by bitcoin-tx), and `CLI_COMMANDS` (used by bitcoin-cli for its non-rpc commands, `-generate`, `-netinfo`, `-addrinfo`, `-getinfo`).

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536254793)
It's a docstring, so shows up in the doxygen docs https://doxygen.bitcoincore.org/class_args_manager.html#ab71d531303f7e6fe3bbf67683a414a2e ; maybe it's not that useful, but doesn't seem worth removing outright

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536256619)
It's looked up by the key in `CheckCommandOptions`, so sorting seems fine to me?

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536272651)
`FormatParagraph()` only indenting when it sees a `\n` seems plausible for dealing with an indented continuation (ie, `"Hello " + FormatParagraph("world!\n", 79, 4)`) and the current behaviour is tested, so I'd rather not touch that.

💬 lovepamthailand1672538 commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3545028414)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q

💬 lovepamthailand1672538 commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3545029569)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536361107)
Not currently, I think

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536364679)
Added some unit test coverage

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3545163344)
> If you can, I would appreciate adding slightly more context to the PR description and the commit messages.

Sorry, these seem completely self-explanatory to me, so I don't see what to add. Happy to answer questions or take suggestions though.

💬 ajtowns commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3545336207)
> Unless we overhaul the sv2 [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) it doesn't help

Yes, I'm saying I think `NewTemplate` (and `CoinbaseOutputConstraints`) should be overhauled, to avoid sv2 software generating or modifying the coinbase, beyond adjusting the values in a reserved extranonce area of known/fixed size. (Parsing the coinbase tx is still necessary for the pool to determine whether to accept a
...

💬 yuvicc commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#issuecomment-3545630672)
Concept ACK

[Wrote](https://github.com/yuvicc/test-bitcoinkernel/blob/master/context_free.c) a small program to test `btck_PrecomputedTransactionData`.

✅ maflcko closed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442)

📝 maflcko reopened a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)

### Summary

The in-memory representation of the UTXO set uses (salted) [SipHash](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L226) for avoiding key collision attacks.

Hashing `uint256` keys is performed frequently throughout the codebase. Previously, specialized optimizations existed as standalone functions (`SipHashUint256` and `SipHashUint25
...

📝 maflcko opened a pull request: "clang-format: Set InsertNewlineAtEOF: true"
(https://github.com/bitcoin/bitcoin/pull/33896)
Now that the minimum supported clang version is 17, the `InsertNewlineAtEOF` setting can be set to `true` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)

This is in line with the already existing newline linter. Can be tested via:

```
truncate -s -1 src/init.cpp
git diff

# Should fail:
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline

# Restore newline:
git diff -U0
...

💬 optout21 commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2536712096)
Yes, a similar check could be done in the beginning, when `tip` is retrieved. But prob. outside of the scope of this PR.

💬 maflcko commented on pull request "guix: update per-host disk-space estimates in build gate":
(https://github.com/bitcoin/bitcoin/pull/33889#discussion_r2536789722)
> I did notice discrepancies for darwin and mingw though, which I've attempted to correct for.

That is fine, but you'll have to properly explain the changes.

https://github.com/kevkevinpal/bitcoin/issues/200 could be an oversight where someone ran the build twice by accident?

💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2536798319)
The call should be fully optimized out anyway, no?

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3546154209)
> OTOH, overhauling NewTemplate shouldn't be a blocker for supporting sv2 efficiently, so Approach ACK for this PR in the meantime.

Thanks, I opened https://github.com/stratum-mining/sv2-spec/issues/167 to continue that discussion.

>> replacing m_options.coinbase_output_script with a vector.

See https://github.com/bitcoin/bitcoin/pull/33890

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2536880741)
Done in the latest push.