💬 brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3085207500)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/33003#discussion_r2213532921

🤔 mzumsande reviewed a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3030644452)
Code Review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50

I think it makes sense not to do these iterations over block txns if these can never result in any action - even if it doesn't increase performance measurably.

💬 mzumsande commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214057025)
nit: If `mapTx` is empty, `mapNextTx` must necessarily be empty as well, so strictly speaking it doesn't need to be checked here.

💬 brunoerg commented on pull request "doc: clarify GetAddresses documentation":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3085283292)
I agree with @stickies-v. It's weird to have two functions with the same name but with a relevant behavior difference, renaming one of the functions seems appropriate. In any case, Concept ACK about improving the documentation.

💬 kurapika007 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3085325955)
> > It's not obvious that the price going up 10x means we get to lower the default by 10x, because it's a more important system now and the stakes are simply higher.
>
> Sure, though at the same time the situation being discussed here isn't price up 10x means default down 10x. Compared to when this was last adjusted the price is up 500x and this proposes down by 10x. :P The magnitudes matter-- particularly in that it answers your drawdown comment: Even if bitcoin prices drop by 90% they'll st
...

👍 brunoerg approved a pull request: "wallet: remove outdated `pszSkip` arg of database `Rewrite` func"
(https://github.com/bitcoin/bitcoin/pull/32990#pullrequestreview-3030870760)
code review ACK 2dfeb6668cb2e98e3dccf946af084e8a08e1fab5

I verified that this arg was used only by the legacy wallet. At 2dfeb6668cb2e98e3dccf946af084e8a08e1fab5 (from the PR that added sqlite), this arg was used in the following function from `BerkeleyDatabase` (which obviously doesn't exist anymore):
```cpp
bool BerkeleyDatabase::Rewrite(const char* pszSkip)
{
while (true) {
{
LOCK(cs_db);
if (m_refcount <= 0) {
// Flush log data t
...

🤔 ismaelsadeeq reviewed a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3030963395)
Concept ACK. I think the comment about updating the fee logic should be removed, as it can easily become stale.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214259323)
Why delete this comment?

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214264010)
Explicitly stating we update fee logic is unnecessary, just state that we fire the `MempoolTransactionsRemovedForBlock` notification or something like that.
```suggestion
// Remove confirmed txs and conflicts when a new block is connected, and notify validation interface listeners
```

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214381461)
Yes, I'm aware, but I have listed here every value that is read or written to - seems safer this way.

🤔 stickies-v reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3031143319)
Concept ACK for removing it.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2214383689)
Or:

> // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.

💬 JeremyRubin commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3085647936)
i'll check if there are others but this looks cleaner

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214389893)
Thanks for the review.
I'm not a big fan of dead comments, especially when they're duplicating information that the code can tell better.
The name already states that it removes from mempool.
And it's called from ConnectTip in validation.cpp.
I have instead added a comment inside the method without repeating the exact same - please see the commit message.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214391892)
The code already states clearly that MempoolTransactionsRemovedForBlock is called.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214400965)
Fair enough, It should be removed then because of the reason here https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3030963395 I've come across multiple places where such comments get stale and not updated.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214403826)
This is a valid point, might be better for such removals to be in it's own commit with this explanation as rationale.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214435971)
Since the method is about mempool removal, I found it surprising that it is needed for fee estimation - hence the comment

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3086471740)
re-ACK dbd76e68c3fd86814e96be85a2d23eb542df6e55

Changes from my last ACK cedbc2cd99754c099e92f074e1d5566da265bf26 (git range-diff 7763e86afa...cedbc2cd99 5d17e64a02...dbd76e68c3)

4480ca4a2f - Added a comment
ead32f37c8 - `const uint256&` in some places were changed to `const Txid/Wtxid&` and this caused a conflict
7255b28cbb - fixed issue with passing 0 as second parameter to sendrawtransaction
9ec10ceb59 - split up combined `if` statement into two separate statements. Expanded on a com
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2214749431)
You did not change it :(. It is commented out so it's not a deal breaker here.

💬 maflcko commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3086779852)
> seems like it got broken multiple times in the past without users really noticing / complaining?

They'd still see the error message about the `-reindex`, before the crash. It seems easy to dismiss a crash as hardware fault and not software fault after you've seen a corruption warning.



> Is there any way to write an actual GUI test for this? Having to add an obscure, single-use option to bitcoind, to simulate crashing behaviour in the GUI, seems like a last resort?

Switched to the
...