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

💬 theStack commented on pull request "Introduce field element and group element classes to test_framework/key.py":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1154452161)
That's a typo I guess?
```suggestion
raising the argument to the power (p + 1) / 4.
```

👋 TheCharlatan's pull request is ready for review: "refactor (tidy): Fixes after enable-debug configure"
(https://github.com/bitcoin/bitcoin/pull/27353)

💬 TheCharlatan commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1491914824)
> I think this PR diff is an improvement.

I'll undraft this then, even if I still don't understand why `clang-tidy` seems to catch more with debug enabled.

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

Could you explain why it's an improvement? It's unclear to me why const references to iterators would be an improvement, I was under the impression that iters by value were fine.

💬 furszy commented on issue "wallet: Data race in GetOrCreateLegacyScriptPubKeyMan vs IsMine":
(https://github.com/bitcoin/bitcoin/issues/27354#issuecomment-1491916385)
> However, I think this may also happen in production, so a proper fix would be to protect m_spk_managers with a mutex? If not, it might be sufficient to add a SyncWithValidationInterfaceQueue() somewhere in the test.

It is hard for this race to occur in production due to the wallet registering to the validation interface after loading, which means that the legacy spkm exists before any chain event takes place. However, other races may still occur. Furthermore, the spkm maps are guarded by `c
...

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491935771)
> https://github.com/bitcoin/bitcoin/blob/328087d16f4362a23a72575fee930a275efbf3dd/src/net.cpp#L1890
>
>
> to determine the `fCountFailure` bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (`nAttempts`). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave `setConnected` unchanged as sdaftuar suggested?

@lightlike Good observation, I missed this. I think that bit of code
...

📝 darosior opened a pull request: "miniscript: explicit cast instead of comparing integers of different signs"
(https://github.com/bitcoin/bitcoin/pull/27382)
Fixes #27381

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1154508362)
I think we should generally avoid adding `assert()` to networking code, unless we're sure that it's really better to crash than to continue, eg because the node is in an inconsistent or corrupt state and is unable to continue running. That doesn't seem to be the case here, so I think we could use `Assume(false)` instead, so that we only get crashes in debug builds and our CI environment.

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1154506161)
As I mention in my comment [elsewhere](https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491935771), I think we can drop this logic.