💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183775132)
You need to make sure that you get *at least* 100 unique coins, otherwise the below loop can do `auto prevout = available_coins.begin();` on empty set, causing undefined behavior(duplicate inputs, in my case).

👍 fanquake approved a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411049490)
nice ACK fa6e2bfd0570324c80677d894247936bc151b4f8

💬 achow101 commented on pull request "wallet: Replace `GetBalance()` logic with `AvailableCoins()`":
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1533154837)
Closing due to lack of interest in this approach.

✅ achow101 closed a pull request: "wallet: Replace `GetBalance()` logic with `AvailableCoins()`"
(https://github.com/bitcoin/bitcoin/pull/26756)

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183794878)
hmm I think the fuzzer was smart enough to give a coin with the same hash as the txid of a constructed transaction below, built on a different set of ancestors...

I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?

👍 hebasto approved a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411070769)
ACK fa6e2bfd0570324c80677d894247936bc151b4f8

💬 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.