💬 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

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

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

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

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

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

Wow, I did not notice that field 🤦. That will make the test a lot better.

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1484756018)
max_selection_weight is also fine I think, both are better than status quo

🤔 sipa reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1873140538)
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484764886)
> re: [#29403 (comment)](https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483514809)
>
> In commit "wallet: bdb batch 'ErasePrefix', do not create txn internally" ([96eb527](https://github.com/bitcoin/bitcoin/commit/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
...