💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2052396373)
nit: could be `const` method.

💬 instagibbs commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#discussion_r2052398185)
Pushed a change to use `MAX_SCRIPT_ELEMENT_SIZE`, but I feel that the code with asserts is already a good explanation?

👍 rkrux approved a pull request: "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)"
(https://github.com/bitcoin/bitcoin/pull/32305#pullrequestreview-2781251768)
tACK 23ee07dd0f81930f46d2f4910cc92cb0eab61cc3

I believe this is a value add as it sets the base for the MuSig2 functional testing. I left few minor suggestions, I would be quick to re-ack if they are implemented.

💬 rkrux commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2052391023)
Since this is a new check that has been added & the current use case is of a list of only bytes (in participant pubkeys), maybe we can assert this on all instead of any to keep the checking strict to start with that can be eased in the future if needed?

💬 rkrux commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2052391602)
Are asserts like on this line really needed? If the decoded psbt is malformed then the test would fail in the following assert anyway.

💬 rkrux commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2052392744)
Nit: Maybe consider putting this in a separate subtest as the current `run_test` is quite long already.

💬 rkrux commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2052395285)
Fine for now but maybe we can consider making an `assert_musig2_fields()` later when more musig2 specific tests are added - if needed though.

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

Simple rebase on top of stability fixes.

`git range-diff master 27a0c93abb7e70b93214eb857e2046f848139e68 d64bd31f9b6903f0a335a78b519d0b52e84cca1b`

💬 Sjors commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052401069)
2be3c59151bcb840eb8cbd045a3dada6775ebc9e: this or the final commit would be a good time to document both `SpendBlock` and `ConnectBlock`.

💬 saikiran57 commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-2818446195)
Hi @GregTonoski
Example:
`./bitcoin-cli.exe -testnet signrawtransactionwithkey 020000000165ef750aac862b0177cadb77961bf1a936e2bec0376b286f5b4e1b6255cf3a960000000000fdffffff012c1a0000000000002251203b82b2b2a9185315da6f80da5f06d0440d8a5e1457fa93387c2d919c86ec878600000000 '["cV628xvqToz45dwdPmTcJ9RgEVnWMwP8dpZBGzb9LfTk3sBHFNwc"]' '[{"txid":"963acf55621b4e5b6f286b37c0bee236a9f11b9677dbca77012b86ac0a75ef65","vout":0,"scriptPubKey":"512055355ca83c973f1d97ce0e3843c85d78905af16b4dc531bc488e57212d230116",
...

💬 GregTonoski commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-2818504035)
Is it related to the bug bitcoin#31589, perhaps?

💬 glozow commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818509467)
I only see 1 ack, ping @instagibbs @0xBEEFCAF3 for re-review

💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818539349)
reACK 62b6071fd6ac2aad2fc42f135b3df662ab0eee7a

removed the assert in favor of the single BOOST_ERROR check

✅ maflcko closed a pull request: "docs: remove passing CI section in guidelines for PR merging as it's common sense"
(https://github.com/bitcoin/bitcoin/pull/32006)

💬 maflcko commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2818560151)
lgtm ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

Makes sense to remove this, because it is common sense, and also redundant with the readme sections https://github.com/bitcoin/bitcoin/blob/master/README.md#automated-testing and https://github.com/bitcoin/bitcoin/blob/master/README.md#manual-quality-assurance-qa-testing.

📝 maflcko reopened a pull request: "docs: remove passing CI section in guidelines for PR merging as it's common sense"
(https://github.com/bitcoin/bitcoin/pull/32006)
Removing passing CI section in guidelines for PR merging as it's common sens

💬 saikiran57 commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-2818575632)
Its not related to bug https://github.com/bitcoin/bitcoin/issues/31589,

you can see this line https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L477 it is consider the XOnlyPubKey{vSolutions[0]} instead of tweaked public key. So that's why when signing https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L67 it returns false because output is locked to tweaked public key where as we are searching for x-only-public key.

👋 mzumsande's pull request is ready for review: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290)

💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818686077)
Concept & Approach ACK

Code nits:
```diff
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -930,11 +930,11 @@ BOOST_AUTO_TEST_CASE(script_json_test)
auto element = test[pos][i].get_str();
// We use #SCRIPT# to flag a non-hex script that we can read using ParseScript
// Taproot script must be third from the last element in witness stack
- std::string scriptFlag = std::string("#SCRIPT#");
-
...

💬 EniRox001 commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2818701974)
> Code change looks good to me. The commit message doesn't seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.

Thanks for the review! I've split the changes into two logical commits as suggested:

1. First commit fixes the nTimes -> nTime typo and explains its impact on the test behavior
2. Second commit addresses the test failure that was
...