💬 danielabrozzoni commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2172253857)
Just to confirm that I understand this correctly, are you removing the `<256>` because it's implicit?

💬 danielabrozzoni commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2180366086)
nit in ced2ddb37c957e53d4a8aa40d3512ea3786511b4: the previous comment (in headerssync.cpp) said:
"Only **feed headers to validation** once this many headers.."

I think that phrasing was a bit clearer than “outputting”

💬 danielabrozzoni commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2180450887)
micro nit in 79bdc6a785bca96dababc698336c04b8625b7d1c: you could check `exp_pow_validated_prev` first and `exp_locator_hash` second, to maintain the same order as the macro parameters

💬 l0rinc commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2180509361)
Yes, they're redundant, can be deduced by the compiler

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3028521384)
@fanquake because it's in the fuzzer code, I forgot to change one of the function signatures there.

👍 darosior approved a pull request: "functional test: correctly detect nonstd TRUC tx vsize in feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32859#pullrequestreview-2979878770)
utACK f0524cda3995cf65adab3d0ca8da0dee4e31c79b

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3028577968)
I probably need to implement this at the `SigningProvider` level rather than `HidingSigningProvider`. The early return needs to be inside `SignTaproot()` right before `// Try script path spending` rather than in `GetTaprootSpendData()`. The goal isn't to hide leaf scripts from co-signers, only to make sure we don't sign them ourselves.

🤔 w0xlt reviewed a pull request: "functional test: correctly detect nonstd TRUC tx vsize in feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32859#pullrequestreview-2979983814)
ACK https://github.com/bitcoin/bitcoin/pull/32859/commits/f0524cda3995cf65adab3d0ca8da0dee4e31c79b

💬 tnndbtc commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3028740646)
Just for document purpose, I profiled `bitcoind` for IBD process on Mac through `gperftool`, and have chatgpt to help analyze out.txt (attached) and following is the summary:

```
LevelDB I/O & compaction ~1500s+ ~30% Block/UTXO writes, frequent flushes
leveldb::DecodeFixed32 — 241.66s (4.93%)
leveldb::DBImpl::DoCompactionWork — 1430.83s (29.2%)
leveldb::TableBuilder::Add — 1276.91s (26.06%)
leveldb::TableBuilder::Flush — 1261.79s (25.75%)

This shows:
Heavy disk write and
...

👍 pinheadmz approved a pull request: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2980079242)
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae

Built and ran all functional tests with --usecli on macos/arm64. Reviewed all code changes again since it's been a while since last review. Coverage is really great.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 666016e56b28b77f798dc85c767b95c1ca0abfae
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmhlbmsACgkQ5+KYS2KJ
yTp9HxAAmFHGtNyp2QfIz2EERL9n8BI/
...

💬 tnndbtc commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3028803853)
@GregTonoski you can email me with my <git user name> @ gmail.com . Then we can schedule a zoom meeting to discuss further.

💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3028819515)
@fanquake Any other thoughts on how to fix this issue, like smaller fixed data set? Currently this test is flawed as it uses random fees and missed the fact that, the randomness will go through different execution path thus the random test failure.

💬 achow101 commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2180720302)
It's already false for existing descriptor wallets.

The purpose of marking it deprecated is so that it can be removed in the future. Otherwise, it may be surprising to users that this field suddenly disappeared.

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3028935370)
Shoehorning this into `SigningProvider` got ugly real fast. So instead I endup up passing `avoid_script_path` along the call stack from `SignPSBTInput` to `SignTaproot`. Which is also ugly...

Well, it compiles and works. Thoughts on how to implement this elegantly?

🤔 glozow reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2979497055)
ACK 2b2df98747fdb6380588991167ce2e8cb92f3bfb

Reviewed the code and the tests. I ran the bench to see that we spend a lot longer on the last commit since we are able to consider more potential clusters (almost 2x the time).

💬 glozow commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180272606)
nit in 2b2df98747f
```suggestion
// We only need to trim the middle bottom transaction to end up with 2 clusters each within cluster limits.
```

💬 glozow commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180717236)
I tested that `Trim()` always removes oversized singletons, even if there are no clusters to be merged, using a simple unit test

```
diff --git a/src/test/txgraph_tests.cpp b/src/test/txgraph_tests.cpp
index 9f9ad04ff37..d90afd8819d 100644
--- a/src/test/txgraph_tests.cpp
+++ b/src/test/txgraph_tests.cpp
@@ -248,4 +248,43 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_huge)
BOOST_CHECK(graph->GetTransactionCount() >= (NUM_TOP_CHAINS * NUM_TX_PER_TOP_CHAIN * 99) / 100);
}

+BOOST_AUTO_TE
...

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3029029658)
> @GregTonoski you can email me with my \<git user name> @ gmail.com . Then we can schedule a zoom meeting to discuss further.

I don't want to talk in private. Let's put all the information in public and solve it together with anybody interested in.

👍 hodlinator approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2980338061)
ACK 705791cd436f237fe9bbac2cf52d63ab4b2a41c7

Fixed typos and improved PR description since my first (concept & test) review (https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2892807188).

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2180801973)
Great, incorporated. I'm assuming the txgraph.cpp changes above are to demonstrate the test works?