💬 ryanofsky commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269529793)
re: https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262333855

> hm see [#27850 (comment)](https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122) not sure what to do

I don't actually understand what this issue is in that comment since the variable seems to be used just once.

It does seem strange to define these commands as variables instead of inlining them when the variables only seem to be used only once. Also would expect them to be shell functions instead of
...

👍 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"`?