💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1277425106)
Dropping a constant but then hardcoding it all over the place with no historical context is not a good idea.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1277425106)
Dropping a constant but then hardcoding it all over the place with no historical context is not a good idea.
🤔 Sjors requested changes to a pull request: "Remove arbitrary restrictions on OP_RETURN by default"
(https://github.com/bitcoin/bitcoin/pull/28130#pullrequestreview-1551912890)
Concept NACK unless you keep the default at `MAX_OP_RETURN_RELAY`. Depending on adoption we can always drop that default later. No need to make pull requests more controversial than necessary.
(https://github.com/bitcoin/bitcoin/pull/28130#pullrequestreview-1551912890)
Concept NACK unless you keep the default at `MAX_OP_RETURN_RELAY`. Depending on adoption we can always drop that default later. No need to make pull requests more controversial than necessary.
🚀 fanquake merged a pull request: "refactor: Revert additional univalue check in ParseSighashString"
(https://github.com/bitcoin/bitcoin/pull/28162)
(https://github.com/bitcoin/bitcoin/pull/28162)
💬 aureleoules commented on pull request "docs: Rewrite README to make it more appealing":
(https://github.com/bitcoin/bitcoin/pull/28174#issuecomment-1655530271)
I understand the points you have raised and do somewhat agree with them. Closing this PR.
(https://github.com/bitcoin/bitcoin/pull/28174#issuecomment-1655530271)
I understand the points you have raised and do somewhat agree with them. Closing this PR.
✅ aureleoules closed a pull request: "docs: Rewrite README to make it more appealing"
(https://github.com/bitcoin/bitcoin/pull/28174)
(https://github.com/bitcoin/bitcoin/pull/28174)
💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1655540354)
With `-mutate_depth=14` being worse (be aware that the y-axis no longer matches to all previous plots)
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1655540354)
With `-mutate_depth=14` being worse (be aware that the y-axis no longer matches to all previous plots)
💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1655543843)
Thanks, generated some data and edited the description to support the `mutate_depth` choice. (Limited to [1,10] for now, but this can be changed back to [1,15], if there is at least one data point showing that values larger than 10 are useful)
(https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1655543843)
Thanks, generated some data and edited the description to support the `mutate_depth` choice. (Limited to [1,10] for now, but this can be changed back to [1,15], if there is at least one data point showing that values larger than 10 are useful)
💬 Sjors commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1655548325)
I guess both changes are a strict improvement, but won't put an end to the whack-a-mole grind.
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1655548325)
I guess both changes are a strict improvement, but won't put an end to the whack-a-mole grind.
💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1277460913)
tiny-nit (or more, a curiosity question): Was it intended to derive the bit position within the byte randomly? Due to the choice of having 8 error values for this case, I was assuming that the idea was to systematically damage with all bit positions 0..7.
```suggestion
to_decrypt[InsecureRandRange(to_decrypt.size())] ^= std::byte(1U << (error - 2));
```
But in practice it shouldn't really matter and I guess both are fine.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1277460913)
tiny-nit (or more, a curiosity question): Was it intended to derive the bit position within the byte randomly? Due to the choice of having 8 error values for this case, I was assuming that the idea was to systematically damage with all bit positions 0..7.
```suggestion
to_decrypt[InsecureRandRange(to_decrypt.size())] ^= std::byte(1U << (error - 2));
```
But in practice it shouldn't really matter and I guess both are fine.
👍 theStack approved a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1551988560)
ACK 1c7582ead6e1119899922041c1af2b4169b0bc74
Finished reviewing. Code looks good to me, checked carefully that the implementation matches BIP324 and verified the test vectors (see above). :heavy_check_mark:
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1551988560)
ACK 1c7582ead6e1119899922041c1af2b4169b0bc74
Finished reviewing. Code looks good to me, checked carefully that the implementation matches BIP324 and verified the test vectors (see above). :heavy_check_mark:
💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1655594515)
@vostrnad's comment makes sense to me. I'll hold off re-reviewing until that's either done or decided against.
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1655594515)
@vostrnad's comment makes sense to me. I'll hold off re-reviewing until that's either done or decided against.
💬 Sjors commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#issuecomment-1655597008)
> If #17487 gets merged I will rebase this
It has been merged.
(https://github.com/bitcoin/bitcoin/pull/15218#issuecomment-1655597008)
> If #17487 gets merged I will rebase this
It has been merged.
💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277494461)
That's a large increase in runtime isn't it? Won't 100k iterations take much less than 100 minutes to complete on most of our targets?
We've got 115 targets at the moments, this change makes it so running this script with `--generate` for all targets would take around 8 days.
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277494461)
That's a large increase in runtime isn't it? Won't 100k iterations take much less than 100 minutes to complete on most of our targets?
We've got 115 targets at the moments, this change makes it so running this script with `--generate` for all targets would take around 8 days.
💬 pinheadmz commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1655625669)
hint: https://github.com/bitcoin/bitcoin/blob/42a9110899a9020417f0397456168aea6ac6ade9/test/functional/feature_taproot.py#L989
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1655625669)
hint: https://github.com/bitcoin/bitcoin/blob/42a9110899a9020417f0397456168aea6ac6ade9/test/functional/feature_taproot.py#L989
💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277507602)
The number of tasks that are spawned is ~300 (`294` for me, but it will likely be more in the future). So even if you run each task only for a minute, it will take longer than 4 hours already. But running for a minute seems entirely useless, so I am not sure if it is worth optimizing for. (For smoke testing I just edit this script to `-max_total_time=6`).
In any way, you can pass `--par 999` to run everything at the same time and be done in 100 minutes.
However, to avoid an edit for smoke
...
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277507602)
The number of tasks that are spawned is ~300 (`294` for me, but it will likely be more in the future). So even if you run each task only for a minute, it will take longer than 4 hours already. But running for a minute seems entirely useless, so I am not sure if it is worth optimizing for. (For smoke testing I just edit this script to `-max_total_time=6`).
In any way, you can pass `--par 999` to run everything at the same time and be done in 100 minutes.
However, to avoid an edit for smoke
...
💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1655662987)
ACK 91f47e676ae3f131e7cecae9b5b5e50b358e9b32
Also tested that reverting 6a9510d2dabde1c9ee6c4226e3d16ca32eb48ac5 causes the test to fail when it tries to load the wallet in master after having loaded it in v25 (`Wallet corrupted`).
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1655662987)
ACK 91f47e676ae3f131e7cecae9b5b5e50b358e9b32
Also tested that reverting 6a9510d2dabde1c9ee6c4226e3d16ca32eb48ac5 causes the test to fail when it tries to load the wallet in master after having loaded it in v25 (`Wallet corrupted`).
💬 darosior commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277548417)
Ok i'm confused. I'm aware i just gave this a superficial look but i'm absolutely not following here. *(Last comment and if i don't get it i'll look into it more in depth.)*
> The number of tasks that are spawned is ~300
How comes? The default for `--par` is `4`.
> So even if you run each task only for a minute, it will take longer than 4 hours already
How could, with 300 tasks, running 169 tasks for 1 minute take 4 hours?
----
My main worry would be that increasing the runtime
...
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277548417)
Ok i'm confused. I'm aware i just gave this a superficial look but i'm absolutely not following here. *(Last comment and if i don't get it i'll look into it more in depth.)*
> The number of tasks that are spawned is ~300
How comes? The default for `--par` is `4`.
> So even if you run each task only for a minute, it will take longer than 4 hours already
How could, with 300 tasks, running 169 tasks for 1 minute take 4 hours?
----
My main worry would be that increasing the runtime
...
💬 MarcoFalke commented on pull request "fuzz: Generate with random libFuzzer settings":
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277559716)
> How comes?
Sorry, with "The number of tasks that are spawned is ~300" I meant "The number of tasks that are spawned over the full runtime of the python script is ~300".
> How could
For example, with `-j1`: Running 300 1-minute-tasks spawned sequentially on one thread will take 300 minutes. (5 hours). Though, the main point is that 1 minute of runtime is useless. So optimizing for a short default runtime will quickly get you to make the entire script useless.
I am happy to change th
...
(https://github.com/bitcoin/bitcoin/pull/28178#discussion_r1277559716)
> How comes?
Sorry, with "The number of tasks that are spawned is ~300" I meant "The number of tasks that are spawned over the full runtime of the python script is ~300".
> How could
For example, with `-j1`: Running 300 1-minute-tasks spawned sequentially on one thread will take 300 minutes. (5 hours). Though, the main point is that 1 minute of runtime is useless. So optimizing for a short default runtime will quickly get you to make the entire script useless.
I am happy to change th
...
💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277560378)
see above.
better boolean notation here would be great.
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277560378)
see above.
better boolean notation here would be great.
💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277557756)
`pop two` doesn't quite work (IMO).
it makes the order somewhat ambiguous.
for example:
`pop pop add push` is more correct. no?
Being more explicit in this way will make the `OP_SUB` make more sense, no?.
`pop pop subtract push`
for `OP_ADD` it isn't much of an issue.
`` 1 + 1 = 2 ``
`` 1 + -1 = 0 ``
`` -1 + 1 = 0 ``
for `OP_SUB` it may be an issue.
``1 - 1 = 0``
``1 - -1 = 0``
``-1 - 1 = -2``
*in common arithmetic notation
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1277557756)
`pop two` doesn't quite work (IMO).
it makes the order somewhat ambiguous.
for example:
`pop pop add push` is more correct. no?
Being more explicit in this way will make the `OP_SUB` make more sense, no?.
`pop pop subtract push`
for `OP_ADD` it isn't much of an issue.
`` 1 + 1 = 2 ``
`` 1 + -1 = 0 ``
`` -1 + 1 = 0 ``
for `OP_SUB` it may be an issue.
``1 - 1 = 0``
``1 - -1 = 0``
``-1 - 1 = -2``
*in common arithmetic notation