💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086650587)
nit: could enforce `sign` as keyword argument in order to enhance readability at the call-sites below
```suggestion
def mine_block_manual(self, node, *, sign):
```

💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086634429)
nit: could import and use the `OP_TRUE` constant from test_framework.script to avoid the magic number

💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086653095)
extra-space-nit:
```suggestion
For custom signets with a trivial challenge such as `OP_TRUE` the walletprocesspsbt step can be skipped.
```

💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086699379)
> If you did indeed have 83 OP_RETURN outputs, then a datacarriersize=83 limit would mean that you'd spent your entire datacarrier budget on that overhead, without actually including any data.

Yes, but isn't it the entire point of the parameter? "to not flood the chain with garbage"

I personally don't care about it, just trying to prevent the brigaders claiming "Core snuck in a backdoor that can blow up the data size".

🤔 Sjors reviewed a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2836498591)
Concept ACK

Mostly happy with 31e3808df9c59d36a07cad07df89ae1bf9e63000

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086664914)
In d3f4e64d416aa29d4448dffb4d47e52997b40271 "interfaces: refactor: move `submitSolution` implementation to miner": maybe say "insert or replace"

It's unlikely for `vtx` to be empty, since we we add a coinbase during template creation.

More importantly, it's possible to call `submitSolution()` multiple times, with each call updating the coinbase. This could happen as a race condition or because the first solution was somehow incorrect.

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086672679)
In 497996c3e572c8992d34e54fd13d92984e9ec21c "interfaces: refactor: move `waitNext` implementation to miner" nit, a little less wide:

```diff
diff --git a/src/node/miner.h b/src/node/miner.h
index e20cab0d08..04b12f065e 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -235,9 +235,17 @@ void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& opti
/* Compute the block's merkle root, insert the coinbase transaction and the merkle root into the block */
void Add
...

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086709335)
In 31e3808df9c59d36a07cad07df89ae1bf9e63000 "interfaces: refactor: move `waitTipChanged` implementation to miner": this neatly solves the deadlock issue, but I think we should preserve that comment.

Additionally we should explain why `getTip()` is called here, ignoring the block hash returned by `WaitTipChanged`. Perhaps the latter should just return a boolean?

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086689234)
In the same commit, it's better to pass you should pass the `CBlockTemplate` as an argument rather than `tx_fees`. With cluster mempool we might overhaul the way we do this fee calculation. Even without that, we might add a helper method to `CBlockTemplate` to get the total fees. In both cases I'd rather not have to change the function signature.

(if you do this, you can also drop the `prev_block_hash` argument)

💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086713176)
Regarding the latter, I see @ryanofsky's point: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2080452135

But we should still explain why we ignore the result.

💬 willcl-ark commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2876362868)
Hey @davidgumberg just getting around to the ping here (sorry).

I have a local manifest which builds 0.16.3 lief: https://gist.github.com/willcl-ark/3ac07722025b696910adba256d813cb6

As with the changes to Lief themselves I began, they def need a second look over, but perhaps it will be of some help.

💬 fanquake commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2876389472)
Guix Build:
```bash
24986a84cc15356c2761a8d05898d33d60ff10431eecdf68d9fb758432a87f79 guix-build-915c1fa72c07/output/aarch64-linux-gnu/SHA256SUMS.part
be7bba66e8e84ba8428aed395a296e397d07f5d961bb8ab80bdd19d2c48589ba guix-build-915c1fa72c07/output/aarch64-linux-gnu/bitcoin-915c1fa72c07-aarch64-linux-gnu-debug.tar.gz
a77553ea1b9eca35f6b9bc00f0be22f3016b8fdf1425b71ec93a6fdbcd8e154c guix-build-915c1fa72c07/output/aarch64-linux-gnu/bitcoin-915c1fa72c07-aarch64-linux-gnu.tar.gz
4f1a82c81d0f9f35
...

💬 laanwj commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2876400295)
Concept ACK it makes sense to be consistent on this, it's kind of annoying if commit A adds a newline at the end, B removes it again, C adds it again etc. On the other hand i hope we can trust (especially random contributors) to know how their editors work and not have to explain it.

💬 theStack commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#issuecomment-2876406989)
Concept ACK

Seems useful to create a pair of valid "same-txid-different-wtxid" transactions for testing purposes.

Didn't try that yet, but I think another possibility to create these without having to involve signatures at all would be a "trivial math puzzle"-like locking script of e.g. "OP_ADD OP_EQUAL 5" (in a p2wsh or p2tr script-path) and swapped witness data elements ([1,4] and [4,1]) in the spending txs.

🚀 fanquake merged a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32299)

💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2876437307)
> I was wondering whether OP_2...OP_16 should also be treated as trivial,

It may be useful to have multiple public signets out there and avoid them accidentally reorging each other.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2876491745)
reACK 8673e8f01917b48a5f5476792f759f44ea49d5a5

`git range-diff master 7208ec2f1f0b9d72e1b4248dae9adbcc5ab625f7 8673e8f01917b48a5f5476792f759f44ea49d5a5`

Consolidated main ordering checks, added explicit variable for tracking end of cluster in blockbuilder, Hand-checked that `m_known_end_of_cluster` is set anytime it is read from, LGTM.

💬 fanquake commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2086824216)
I don't think we are going to merge changes here in regards to setting architectures etc.

💬 stickies-v commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#discussion_r2086831091)
You should only be counting files tracked by git: `git ls-files | sed -n 's/.*\.//p' | sort | uniq -c | sort -rn`

👍 hodlinator approved a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2836787793)
re-ACK c0f41523973e33a8cd104a8ebf4906a4d99f3eed

Changes since [initial ACK](https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2826464805):
* Rebased to handle file move: contrib/devtools/symbol-check.py -> contrib/guix/symbol-check.py