💬 achow101 commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#issuecomment-1936426651)
ACK 9a3c5c8697659e34d0476103af942a4615818f4e

The new code is much simpler.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1936428779)
ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1936432223)
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1936440669)
tACK [c10f528](https://github.com/bitcoin/bitcoin/pull/27114/commits/c10f528350152ca9248e8c167b67993fc7321ca3)

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484694946)
Ah, I see. That's unfortunate as it means this optimization isn't available before some solution was found, but probably doesn't matter much.

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484695646)
Oh, I missed that this was just changing the expected result, not the input. Sounds good.

💬 ManuelSchneid3r commented on issue "Weird focus rect displayed on inital sync":
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1936463536)
I just started it. yes Actually I deactivated the app to let the focus disappear.

🤔 mzumsande reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1871315375)
Concept ACK
This looks like a nice simplification.

💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484708844)
The next paragraph refers to this deleted comment ("using
the nStatus flag mentioned above") and should be updated too.

💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484705186)
Is it necessary to keep a placeholder / would just removing it break something? I'm asking beacuse as far as I can see, the `enum BlockStatus` is only used in `IsValid` / `RaiseValidity`, but not for `nStatus`, which is a `uint32_t`.

💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483625151)
Why check these values indirectly instead of directly (field `txcount`)?

💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1483671675)
comment can be removed, it's no longer faked.

💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484707431)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (b6dcfca8f77f32cb1f41230fb71bab6666252041)

I think it's bad for debugging that this function inconsistently logs errors when an operation fails. For example it seems like there is no error if the new eraserecords function fails? It would be better to add `LogPrint(BCLog::WALLETDB, "Error: %s failed\n", process_desc);` here.

I understand that in the future there may be cases where we may want to abort without logging errors
...

👍 ryanofsky approved a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1873039395)
Code review ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249. Looks pretty good, but would suggest cleaning up some things

💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484725360)
re: https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514809

In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" (96eb527fad0effe082b087c0898843feccc2e213)

The new comment "Additionally, read-write operations performed outside a db txn context are disallowed by current BDB configurations" seems vague to me and I don't think would be comprehensible without reading this review thread. Would suggest just using achow's comment or combining both comments with s
...

💬 ryanofsky commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484711516)
In commit "wallet: db, introduce 'RunWithinTxn()' helper function" (b6dcfca8f77f32cb1f41230fb71bab6666252041)

Would be useful to add this is determines whether to commit or roll back the txn

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737221)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737288)
done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737478)
added note

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484737550)
done