👍 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.
(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`.
(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)
```
(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.
(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)`
(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 =
...
(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.
(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)
(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
(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)
(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?
(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.
(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.
(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
...
(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.
(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.
(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.
```
(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)
(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.
(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.
(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.