💬 billymcbip commented on issue "Add unit tests for Taproot code in src/script/interpreter.cpp":
(https://github.com/bitcoin/bitcoin/issues/23279#issuecomment-3586837302)
> SigVersion::TAPSCRIPT code paths in ExecuteWitnessScript

I've added a unit test for the empty pubkey error in Tapscript signature validation: https://github.com/bitcoin/bitcoin/pull/33961.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569586837)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569596852)
Done

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569597731)
> why not use them

https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709821764
https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1923995451
https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559477612

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569600697)
Done

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569603708)
you will have a conflict where you're making Flush abstract and they're making it void

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569611664)
Added new lines.

> and sort the groups for predctability

hmm?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569620435)
Added `-i2psam=` to make the I2P network reachable.

🤔 fjahr reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3515236075)
tACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2

Tested this with my own Signet snapshot and with and without pruning.

Some of the renamings could be even more descriptive for my taste but this is a big step forward, so no need to address my nits unless you really want to. If others like them they could still be applied in a simple scripted-diff follow-up.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568688269)
in a162863f2796cb4a5f15f2e80b8e51e7de9cfb50

nit: For me "current" isn't clearer than "active" and I'm very used to active, so I am kind of -0 on this particular name change. Maybe `MostWorkChainstate` or `TipChainstate` would make explicit what is currently written in the comment.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568652812)
in 21d638c92f56f5ad4d32c47240700b41e8868789

nit: How about `maybe_historical_target_stats` 🙈 It's not validated until later and the maybe refers to `std::optional` afaict.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568629658)
in 1107e04328735abe636692b1a0179c6e46613d4e

micro-nit: one of these you moved over to curly brace notation but not the others.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2568578902)
in 0cb19b78cc297ceca591ee8be6f63b4ea7b494cc

nit: Might have been possible to get to a naming that makes these two values even more aligned. Something like `m_snapshot_base_blockhash` and `m_historical_target_blockhash`. I guess in general I would prefer if the "Target" functions would be named "HistoricalTarget" so it's clear these are assumeutxo functions. But it's also a bit too verbose for my taste. So feel free to ignore.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569212831)
in 21d638c92f56f5ad4d32c47240700b41e8868789

nit: `DetectUnvalidatedChainstate` would have been more consistent with the rest of the naming choices in this commit

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569487654)
in f85ff75dad4ce8712fe65a636cf36d57b4066785

If `RemoveChainstate` returned a `nullptr` this would crash. Maybe not a scenario that can be realistically hit currently but might still be good to handle it explicitly here or by changing `RemoveChainstate`.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2569493043)
in d4cbeaff4ae82203b588c3d5b9da011e4bb644d2

nit: Could have added a comment

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2569633692)
The first link shows a benefit to using them, and that is now in our codebase.
The second and third links reference [this blog post](https://blog.aaronballman.com/2020/08/dont-use-the-likely-or-unlikely-attributes/) which has crazy convoluted usages. I don't find that blog post convincing. The usages here are straightforward and in very hot paths. They are only used in paths where we know a valid block will always or never go into (aside from BIP30).

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569633840)
like with the includes, having a predictable order (e.g. alphabetic) can help with merge conflicts and are just generally less opinionated. Ignore if you think it doesn't apply here.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569644851)
Taken. `-listenonion=0` is not needed for the first test, removed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3586923032)
`50c8320f20...e0c9a75fc1`: address suggestions