💬 dergoegge commented on pull request "net processing: #26140 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/27379#discussion_r1154414721)
Done

💬 MarcoFalke commented on issue "miniscript: comparison of integers of different signs":
(https://github.com/bitcoin/bitcoin/issues/27381#issuecomment-1491854885)
It should be fine to make the returned value of `std::count` unsigned.

💬 darosior commented on issue "miniscript: comparison of integers of different signs":
(https://github.com/bitcoin/bitcoin/issues/27381#issuecomment-1491856229)
I'm on it. I'll check if there are other occurrences too, had quite a lot of them in #27255.

💬 MarcoFalke commented on pull request "test: Remove python3.5 workaround":
(https://github.com/bitcoin/bitcoin/pull/27378#discussion_r1154418992)
thx, done

👍 Sjors approved a pull request: "script: add description for the functionality of each opcode"
(https://github.com/bitcoin/bitcoin/pull/27109)
ACK 40f36d0a3dac568bc9b05dea021d83986d8add84

It matches my (limited) understanding of what these opcodes do. I also walked through `EvalScript()` to compare.

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154337691)
Note to other reviewers: this is implemented in `CastToBool()` in `interpreter.cpp`, which is used by `OP_IF`, `OP_NOTIF`, `OP_VERIFY`, `OP_IFDUP`, `OP_NUMEQUALVERIFY` and after executing the `scriptPubKey`.

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154247024)
Can be added in a followup if you don't want to lose ACKs:

```cpp
OP_CHECKSIGADD = 0xba, // pop the public key, N and a signature, push N if signature is empty, fail if it's invalid, otherwise push N + 1 (see BIP 342)
```

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154350939)
Could add: `is true (1 for tapscript)`

Although we probably don't want to go into too much detail here, this is still useful to point out since it directly contradicts the `true` / `false` explanation at the top.

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154351337)
Could add: `is false ("" for tapscript)`

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154357244)
Note for other reviewers, the [Bitcoin Wiki](https://en.bitcoin.it/wiki/Script) provides a useful hint on how to read Satoshi's comments in `interpreter.cpp`:

> the inputs and outputs are both described by items as if they were pushed on the stack in that order. So for example, "x1 x2" indicates pushing value x1 on the stack, then x2, such that x1 is at the bottom of the stack, and x2 is at the top

This also explains why items from the stack are read "out of order":

```
valtype vch1 =
...

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1154398410)
I still have no idea how `OP_CODESEPARATOR` works, but that will take more than one code comment to explain.

💬 dergoegge commented on pull request "addrman, refactor: improve stochastic test in `AddSingle`":
(https://github.com/bitcoin/bitcoin/pull/27319#issuecomment-1491872681)
This doesn't seem like a big win. Have you benchmarked this to see if it actually makes a difference?

(fwiw I think the bigger optimization you are making here is using `<<` over the loop, as opposed to what you mention in the description)

💬 dergoegge commented on pull request "net processing: #26140 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/27379#issuecomment-1491873671)
CI failure is intermittent, see #27380

✅ fanquake closed a pull request: "util/system: Close non-std fds when execing slave processes"
(https://github.com/bitcoin/bitcoin/pull/22417)

💬 MarcoFalke commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1491881510)
Can an admin enable check suites for the webhook in all repos, please?


💬 fanquake commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1491884311)
> enable check suites for the webhook in all repos, please?

Should be available now.

💬 hebasto commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1491884798)
> Besides, I'm also not sold that this lint is an actual improvement.

I think this PR diff _is_ an improvement.

💬 brunoerg commented on pull request "addrman, refactor: improve stochastic test in `AddSingle`":
(https://github.com/bitcoin/bitcoin/pull/27319#issuecomment-1491885088)
> (fwiw I think the bigger optimization you are making here is using << over the loop, as opposed to what you mention in the description)

Just updated the description, thanks.

> This doesn't seem like a big win. Have you benchmarked this to see if it actually makes a difference?

In a general view perhaps we won't see a HUGE difference, didn't benchmark to ensure. However, it changes this algorithm from O(n) - where `n` is the value of `pinfo->nRefCount` - to O(1). Since this is a simple
...

💬 fanquake commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1491885670)
> Use [drahtbot](https://github.com/marcofalke/drahtbot) to tag the author as assignee when CI fails (and remove them as assignee upon force push)

Let's not do this, because it makes assignment potentially useless otherwise.

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1491886629)
Thanks, @stickies-v for your review. Will address your latest suggestions in a follow-up PR.