🤔 pinheadmz reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3573153184)
code-review ACK up to a57ef7ddbacebf23c4c50c3fc58615a58339dcc2 (about 2/3rds through). Also built on macos/arm64 and ran unit and functional tests.

Added a few non blocking nits.

Will finish review next week, re-test in warnet, and play with the feature in the GUI ;-)

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

nit: suggestion
`- Mark that given recipient node has confirmed receipt of a transaction`

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

If we get to this point, the log message will be re-printed fairly rapidly until an OK address is found and resets the counter?

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615351368)
...although, we check `IsRoutable()` when adding to AddrMan so I think it's more likely this code never executes. (Uncovered by tests, too: https://corecheck.dev/bitcoin/bitcoin/pulls/29415)

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

nit: should this be `[in,out]`?

💬 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.