👍 ryanofsky approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539378350)
Code review ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6. This adds nice clear tests and a more realistic CI environment.

💬 achow101 commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644029992)
How big is too big?

`wallet.cpp` is pretty much entirely `CWallet`'s members, so breaking it up will require breaking up `CWallet` which I'm not sure we want to do? I suppose not all of the members need to be in `CWallet`, but I think it'll still be necessary for that class to be kinda big.

🚀 fanquake merged a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620)

💬 theuni commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644049013)
> How big is too big?

Good question :)

>
> `wallet.cpp` is pretty much entirely `CWallet`'s members, so breaking it up will require breaking up `CWallet` which I'm not sure we want to do?

Yeah, agreed that we shouldn't be breaking up classes for something like this. I suspect this actually has more to do with the _giant_ list of includes. Specifically, getting rid of boost signals ([here's a POC from a while back](https://github.com/theuni/bitcoin/commits/replace-boost-signals)) would
...

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1269574745)
Fixed.

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1269576094)
That'd mean grabbing the lock twice, no?

👍 darosior approved a pull request: "descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1539461940)
utACK dd9633b516d6936ac4e23a40f9b0bea120117d35

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644073315)
@psgreco See above; it turned out that what I intended to do here wasn't actually what was implemented (it was instead unconditionally preferring receive over send). Would you mind trying again if this fixes the issue for you?

💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1269593154)
> so using the same key in two emplace calls would cause the second to overwrite the first.

Just for the record: It's the other way round, the second call gets ignored and nothing overwritten, see https://en.cppreference.com/w/cpp/container/unordered_map/emplace

💬 MarcoFalke commented on issue "ci: failure in Docker build step":
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1644095471)
Jup, this was fixed upstream

💬 achow101 commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1644102859)
ACK dd9633b516d6936ac4e23a40f9b0bea120117d35

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269594285)
Suggestion: "mark transaction invalid."

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269592881)
In the text below it seems somewhat arbitrary whether opcodes have an effect when appearing in unexecuted branches. I suggest instead adding something like this up front:

```
* Unless specified otherwise, all opcodes below only have an effect when they occur in executed branches.
```

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269596736)
More precise: `(exactly "\x01" for tapscript)`

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269595880)
Suggestion : "mark transaction invalid"

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269600035)
Suggestion: "mark tx invalid, even when occurring in unexecuted branch" (here and below), for consistency with language elsewhere.

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269597677)
Suggestion: "mark tx invalid"

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269603850)
Use `"\x01"` and `""` instead of 1 and 0 (as 1 and 0 are interpretations, which have multiple possible encodings).

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269607781)
"return" is imprecise I think. How about `Replace top stack item by "" if its value is 0, otherwise by "\x01"`?

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269605109)
"mark tx invalid even when occurring in unexecuted branch"