💬 achow101 commented on pull request "test: add test for corrupt wallet bdb logs":
(https://github.com/bitcoin/bitcoin/pull/20974#issuecomment-1533164196)
I'm not sure that the issue this is trying to fix is really an issue, especially since we have BDB slated for removal.

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797662)
Fixed to `!=`

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797967)
Added a commit dropping those.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183797966)
I was unclear with the word "stateless". What I meant was, "does not retain memory of previous interactions", e.g. we set all the options (including the consensus params) on initialization.

💬 ryanofsky commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1533184415)
> is there some special information in the log you are writing that we can't get from the debug log file?

I think so. The json log captures all inputs to fee estimator to used update its internal state. It includes not just mempool events but also information about which transactions are included in blocks, flush events, and contents of `fee_estimates.dat` blobs that are loaded. It makes fee estimation code reproducible, so if you make a change to the fee estimation code or parameters, you ca
...

💬 achow101 commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183812824)
Sorry, I think I gave the wrong commit. It should be a936a3f194f855e269a4dc2c25e17dbc55eb7a18 "tests: Run both descriptor and legacy wallet modes in single invocation".

The tmpdir changes are needed for this commit because the entire test is restarted for the other wallet type, so it needs to be able to make a new datadir.

💬 achow101 commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#issuecomment-1533196488)
> Can you update the description detailing the approach you are now taking?

Done.

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183810238)
In 45e38158:

Could also remove the `DatabaseOptions` var by providing the `create_flags` uint64_t directly to `BenchLoadWallet`.

And remove the `using wallet::DatabaseFormat` at the top to make the CI happy.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183816539)
I copied this behavior from `ChainstateManagerOpts`. We expect the default constructed `Options` to contain defaults. If we don't have defaults, we assign on object construction. So, yes, in both instances a `const` qualifier (and maybe in others using the same pattern) should be added.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183817443)
Oops, fixed.

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183821846)
Done

💬 achow101 commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#issuecomment-1533214936)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0

🚀 achow101 merged a pull request: "wallet: Refactor and document CoinControl"
(https://github.com/bitcoin/bitcoin/pull/26066)

🤔 theStack reviewed a pull request: "wallet: fix deadlock in bdb read write operation"
(https://github.com/bitcoin/bitcoin/pull/27556#pullrequestreview-1411142149)
Didn't look closer at the changes yet, but can confirm that running the newly introduced unit test via
```
$ ./src/test/test_bitcoin --log_level=all --run_test=walletdb_tests/walletdb_read_write_deadlock
```
succeeds on the PR branch head and hangs if the bugfix commit bda562ddba13ee8f94147734ab9901ab07357f2f is reverted.

👍 furszy approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1411147768)
ACK 00b6e0bb

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183848909)
Note:
If this is used in a read-write process, I'm quite sure that it will suffer from the same problematic tackled in #27556.

💬 achow101 commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1533259027)
ACK d3419f4108669d37a4ccecab77750e70c2f6237c

💬 achow101 commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1533261116)
Backport shouldn't be required as I don't think there's any user action that can trigger the deadlock.

💬 achow101 commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1183870974)
We should just make the `batch` argument required to avoid this kind of issue in the future.

💬 furszy commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#discussion_r1183886841)
sure

🤔 glozow reviewed a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1411212761)
Addressed comments, rebased, added a release note