💬 furszy commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183690806)
Performance wise, create a new thread per lookup is quite bad. Should use a worker pool instead.

I have implemented a generic one for #26966 (https://github.com/bitcoin/bitcoin/pull/26966/commits/f448668f473675915fcebf0e1bd3d73dcb1881af) that could also use here. It also support callback futures so you can remove the costly active wait as well.

Give it a look and if you like it, could decouple it into a standalone PR so we could start using it in both PRs.

📝 MarcoFalke opened a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562)
This allows to cache the image and thus speed up the CI task

👍 Xekyo approved a pull request: "wallet: Refactor and document CoinControl"
(https://github.com/bitcoin/bitcoin/pull/26066#pullrequestreview-1410939913)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0

💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183713583)
Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change `UnSelect` to `Unselect`. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.

```suggestion
void CCo
...

💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183714494)
Nit with the same reasoning as above:
```suggestion
void CCoinControl::DeselectAll()
```

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183738245)
> I wonder if the for loop can be removed, and replaced by the shutdown workflow from https://github.com/bitcoin/bitcoin/pull/26674#issuecomment-1345237799

That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.

The change I pushed will be reverted. How about instead of moving the previously `static` functions of `blockstorage.cpp` into its `BlockManager` class, moving them to a stateless sub-class of the `BlockManager`? Then we can have the
...

💬 MarcoFalke commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183754343)
> They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".

I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to `TestHandler`. `TestHandler` should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its
...

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183761759)
I very much like them to be stateful, see also https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059 . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183762778)
This cleanup applies to other functions as well, so I am happy to have this split up, to avoid this pull from growing too large?

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183773763)
In f76830e5:

This db cursor changes need to be in the first commit as it's not compiling currently.

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