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

💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269598813)
I think it would be useful to use a notation like `[ ... x0 x1] -> [... x0 x1 x0 x1]` for these opcodes instead of trying to explain in English.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269613973)
> that'll certainly be faster but then we can't verify a successful reindex in the new run-as-root path.

Any reason why it wouldn't be able to reindex?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1644114670)
lgtm ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6 🥛

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 08b275c6d27f06cd
...

👍 ryanofsky approved a pull request: "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count"
(https://github.com/bitcoin/bitcoin/pull/27511#pullrequestreview-1539500332)
Code review ACK 7d7ee5e0f4254495349ea68f3d0127c0c336fd12

💬 ryanofsky commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269601639)
In commit "rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table" (332cf61a5cdf838a20117280dc2d24659a4b4a5c)

It seems inaccurate to say "RPC is for testing only" when PR description says "This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman" which makes it sound like this is useful for troubleshooting or monitoring.

If the RPC is not intended just to test the software, I think it
...

🚀 achow101 merged a pull request: "descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067)

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269620812)
ah ya know what I didn't read your comment correctly I thought you were suggesting filling the blocks with invalid junk but that line with `generateblock()` is cool

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261648754)
hmmm, this is a `txid`, not a `wtxid`, which is essentially a no-op unless it's a non-segwit tx. Could we instead call `MempoolRejectedTx` at the point where `MEMPOOLREJ` logging is done, when we have access to the wtxid, since that's the case we actually care about?