💬 pablomartin4btc commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2615354482)
nit (if you have to retouch and agree):
```suggestion
#define detail_LogIfCategoryAcceptsLevel(category, level, ...) \
```
or `detail_LogIfCategoryIsAcceptedAtLevel`... perhaps it's clearer and more consistent with existing internal call API `LogAcceptCategory()`.

🤔 pablomartin4btc reviewed a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3573326964)
ACK fa33a9f5be1beabfee6c217d133cc51dea3b9a63

I agree with the benefits mentioned in the PR description.

⚠️ husstege-e opened an issue: "Wallet: gettransaction(txid).details changes after confirmation of RBF replacement"
(https://github.com/bitcoin/bitcoin/issues/34064)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

For a replaced transaction (txA), the output of gettransaction(txA, true).details
changes after the replacement transaction (txB) is confirmed.

Specifically, a 'receive' entry present after bumpfee disappears once txB is
confirmed, even though the queried txid remains txA.

After confirmation, repeated calls are stable.


### Expected behaviour

For a given txid, gettransaction(txid).deta
...

🤔 pablomartin4btc reviewed a pull request: "lint: Remove confusing, redundant, and brittle lint-spelling"
(https://github.com/bitcoin/bitcoin/pull/34053#pullrequestreview-3573513114)
ACK fa904fc683c0892eb800ddb986fdc0c646721077

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2615504333)
Yes, that's also my understanding. I'm working on a PR that will resolve this.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615528552)
If I understand it correctly it's just wasted work in rare cases - in which case I agree we should tackle it in a follow-up.

🤔 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