💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615548852)
This is already mentioned in a comment about a different part, but applies here as well. A variation of this comment could be added here.

https://github.com/bitcoin/bitcoin/pull/29415/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1644-R1645

🤔 instagibbs reviewed a pull request: "Replace cluster linearization algorithm with SFL"
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-3564457615)
reviewed through ab1416bb471c9326bb77ee464be6aeebf663e024

continuing on with optimizations next

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611075318)
1c008c063df383aff64c564b116802b427a228ce

```Suggestion
void TestOptimalLinearization(const std::vector<uint8_t>& enc, const std::vector<FeeFrac>& optimal_diagram)
```

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2608193810)
8f0828ea5144f261ff5e0cc39ee5eed17d0eec57

why this reduction? commit message commenting on it could be helpful

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611100880)
1c008c063df383aff64c564b116802b427a228ce

Please leave a note that 1M iterations is enough for all given dependency graphs but often a lot less than MaxOptimalLinearizationIters

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611720199)
c2eaf22503f7af057599f02ecb5f24a267ea33e2

Exposing my ignorance here, but why reserve this value? Deserves a comment.

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615138367)
```Suggestion
// Deactivate it otherwise and then make it topological with a series of merges.
```

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611171333)
094e845f7309c8cf4e172c8fe07e200e054ca2db

TIL, nice

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611697771)
c2eaf22503f7af057599f02ecb5f24a267ea33e2

suggestion:
```
- /** The set of all transactions in the cluster. */
- SetType m_transactions;
- /** Information about each transaction (and chunks). Indexed by TxIdx. */
+ /** The set of all TxIdx's of transactions in the cluster indexing into m_tx_data. */
+ SetType m_transaction_idxs;
+ /** Information about each transaction (and chunks). Keeps the "holes" from DepGraph
+ during construction. Indexed by TxIdx. */

...

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2614950800)
a0cb73b4257f771353f6732e7afdaf2d03b51efe

now that UpdateChunk has two TxIdx args let's annotate themhere and other three spots

💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615158883)
a0cb73b4257f771353f6732e7afdaf2d03b51efe

IIUC these two invocations are optimizations we can use since we know where things are potentially mergable next instead of `MakeTopological`?

💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615582635)
done

💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615583174)
After [this discussion](https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472) in which we discussed the separate topic that the current `InvalidChainFound()` call in `ActivateBestChainStep()` often does repeated work and logging, I decided to drop the commit, and will work on a PR to reorder things instead. Current rough plan is to move most things from `InvalidChainFound` to `InvalidBlockFound` and rename the former to `UpdateBestInvalid()`.

For this PR, I have instead added
...

💬 mzumsande commented on pull request "doc: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615586804)
I added a sentence with the basics (didn't include the describe the different orderings).

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615619523)
> can this result in an infinite loop or disconnect/reconnects?

@l0rinc hmm possibly. The next connection will get a `nullopt` `opt_tx` in `PushPrivateBroadcastTx` and disconnect, and then here when finalizing it will return `false` for `DidNodeConfirmReception` again and keep reopening. I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.
```suggestion
if (node.IsPrivateBroadcast
...

💬 l0rinc commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#issuecomment-3648279574)
It seems you've hit https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117501
It seems we can fix it with an explicit `constexpr` for the declarations, see: https://godbolt.org/z/xb5TMaPs6

I have pushed an alternative to https://github.com/l0rinc/bitcoin/pull/63, please cherry-pick it if it passes CI.
Note that I've also changed some constructors to use brace init and extended the commit message with the issue and the godbolt reproducer.

💬 l0rinc commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2615700914)
> added you as a commit co-author

You need to add a [coauthor](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#creating-co-authored-commits-on-the-command-line) with a fixed format, e.g. https://github.com/bitcoin/bitcoin/commit/8e4c66d0a7a0911c10dced0d6dd60ca7bd9545af

💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2615702010)
Duration works as an atomic fine -- we have `atomic<seconds>` and `atomic<microseconds>` in various places in net, net_processing and util/time. `atomic<time_point>` doesn't work because even if the duration is `noexcept` (necessary for atomic), that doesn't propagate to the time_point's constructor also being noexcept which is a requirement for wrapping in atomic.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615782689)
> I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.

That makes sense. Having additional attempts that would be spent on other transactions in the worst case is no big deal in my opinion, but obviously the infinite loop possibility should be fixed.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788225)
I have reverted this change. My benchmarks show that the time per cost is quite similar to before, which I'm now mentioning in the commit message. This will be overhauled in #34023.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788367)
Done.