💬 jnewbery commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532970517)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183644247)
What about the db `require_format` that is above? Is that used anywhere?

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183652859)
Thank you for the context, I see that I am conceptually going backwards here.

💬 fanquake commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1533030631)
> I thought this branch might be preferable...

If we are going to add a script like this to the repository, it should be incorporated into the release process (guix build). AM thinking about how best to integrate it into that process, if we want to inspect the just-compiled .o files, and if there is another way to perform this test.

cc @laanwj you might have some thoughts here?

💬 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