💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2092955734)
@sipa
> What is the issue with the ... miniscript fuzz tests?
The miniscript issue is resolved with your https://github.com/bitcoin/bitcoin/pull/28657 :)
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2092955734)
@sipa
> What is the issue with the ... miniscript fuzz tests?
The miniscript issue is resolved with your https://github.com/bitcoin/bitcoin/pull/28657 :)
💬 ajtowns commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2092959394)
> I'd be curious to see what that patch look like.
Something along the lines of https://github.com/bitcoin/bitcoin/commit/970c0c31e99bebe2b60ade930c96600d1d82f7ca maybe? Essentially untested.
The main advantage of this is that block 0 in a retarget period could also be min difficulty, allowing you to keep mining min-difficulty blocks with 20 minutes between them, halving the difficulty every 4 weeks / 2016 blocks, until you get to something that the network can mine on top of. The disadvan
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2092959394)
> I'd be curious to see what that patch look like.
Something along the lines of https://github.com/bitcoin/bitcoin/commit/970c0c31e99bebe2b60ade930c96600d1d82f7ca maybe? Essentially untested.
The main advantage of this is that block 0 in a retarget period could also be min difficulty, allowing you to keep mining min-difficulty blocks with 20 minutes between them, halving the difficulty every 4 weeks / 2016 blocks, until you get to something that the network can mine on top of. The disadvan
...
👍 hebasto approved a pull request: "miniscript: make operator""_mst consteval"
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038082509)
ACK 73619b819c37e98bd9a100bea14a3366aebfabb8.
I apologise for messing up.
The https://github.com/bitcoin/bitcoin/pull/30031, which is built on top of this PR, succeeds. It includes additional code changes in `src/test/fuzz/miniscript.cpp` (with some inconsistency in local variable naming).
@sipa feel free to pull changes from #30031 into this PR.
(https://github.com/bitcoin/bitcoin/pull/28657#pullrequestreview-2038082509)
ACK 73619b819c37e98bd9a100bea14a3366aebfabb8.
I apologise for messing up.
The https://github.com/bitcoin/bitcoin/pull/30031, which is built on top of this PR, succeeds. It includes additional code changes in `src/test/fuzz/miniscript.cpp` (with some inconsistency in local variable naming).
@sipa feel free to pull changes from #30031 into this PR.
💬 GregTonoski commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2092984943)
What is the meaning and the purpose of the both the "520" and the "MAX_SCRIPT_ELEMENT_SIZE"? In particular, is the "520" and "MAX_SCRIPT_ELEMENT_SIZE" meant to describe limit of size of any element in order to disregard transactions that would have overused resources (similarily to MAX_SCRIPT_SIZE albeit on a level deeper)?
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2092984943)
What is the meaning and the purpose of the both the "520" and the "MAX_SCRIPT_ELEMENT_SIZE"? In particular, is the "520" and "MAX_SCRIPT_ELEMENT_SIZE" meant to describe limit of size of any element in order to disregard transactions that would have overused resources (similarily to MAX_SCRIPT_SIZE albeit on a level deeper)?
💬 TheCharlatan commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2092997971)
Guix builds (aarch64)
```
a416b91cfcc2ed18250021e67b5130e46d407998a642cdaed49996c0be79dd08 guix-build-f0e22be69a15/output/aarch64-linux-gnu/SHA256SUMS.part
569dc4cf491fe4f07009ce8f9d202c05f29a18eec6963fa3e7286a944dd93a82 guix-build-f0e22be69a15/output/aarch64-linux-gnu/bitcoin-f0e22be69a15-aarch64-linux-gnu-debug.tar.gz
126a3b9712bb7685e89e84f6b0d18cce6389bf580e3e66136a976e1f2f50f76f guix-build-f0e22be69a15/output/aarch64-linux-gnu/bitcoin-f0e22be69a15-aarch64-linux-gnu.tar.gz
90a7866982
...
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2092997971)
Guix builds (aarch64)
```
a416b91cfcc2ed18250021e67b5130e46d407998a642cdaed49996c0be79dd08 guix-build-f0e22be69a15/output/aarch64-linux-gnu/SHA256SUMS.part
569dc4cf491fe4f07009ce8f9d202c05f29a18eec6963fa3e7286a944dd93a82 guix-build-f0e22be69a15/output/aarch64-linux-gnu/bitcoin-f0e22be69a15-aarch64-linux-gnu-debug.tar.gz
126a3b9712bb7685e89e84f6b0d18cce6389bf580e3e66136a976e1f2f50f76f guix-build-f0e22be69a15/output/aarch64-linux-gnu/bitcoin-f0e22be69a15-aarch64-linux-gnu.tar.gz
90a7866982
...
💬 willcl-ark commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093006666)
> Is there an easy way to find those?
I was using this https://github.com/becheran/mlc/ in offline mode to find broken internal links. I'm sure it wouldn't take much to modify it to surface relative links, if we want them all absolute?
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093006666)
> Is there an easy way to find those?
I was using this https://github.com/becheran/mlc/ in offline mode to find broken internal links. I'm sure it wouldn't take much to modify it to surface relative links, if we want them all absolute?
💬 theuni commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093030865)
I can't repro the `-Wmaybe-uninitialized` issue. Might it be possible to just add the initializations (even if they're unnecessary) rather than disabling?
Like for the example provided, making it:
```c++
T result{};
```
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093030865)
I can't repro the `-Wmaybe-uninitialized` issue. Might it be possible to just add the initializations (even if they're unnecessary) rather than disabling?
Like for the example provided, making it:
```c++
T result{};
```
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093037407)
Here's a commit that makes `getblocktemplate` reorg low difficulty testnet4 blocks: https://github.com/Sjors/bitcoin/commit/2125fc56163ceaddfadbc78ad0d0da5b1a99a8fa It adds some complexity, but not the validation code.
Untested, other than the the template looks correct to me. I still need to figure out an easy way to setup a local stratum v1 pool (to CPU mine).
By the way, I noticed that `src/bitcoin-cli -testnet4 -generate 1 1000000000` does not find a block if called a dozen times, even
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093037407)
Here's a commit that makes `getblocktemplate` reorg low difficulty testnet4 blocks: https://github.com/Sjors/bitcoin/commit/2125fc56163ceaddfadbc78ad0d0da5b1a99a8fa It adds some complexity, but not the validation code.
Untested, other than the the template looks correct to me. I still need to figure out an easy way to setup a local stratum v1 pool (to CPU mine).
By the way, I noticed that `src/bitcoin-cli -testnet4 -generate 1 1000000000` does not find a block if called a dozen times, even
...
👍 theuni approved a pull request: "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set"
(https://github.com/bitcoin/bitcoin/pull/25972#pullrequestreview-2038169127)
ACK f0e22be69a15248c42964d57f44ce8c37a36081d. It'll be nice to have this fixed.
(https://github.com/bitcoin/bitcoin/pull/25972#pullrequestreview-2038169127)
ACK f0e22be69a15248c42964d57f44ce8c37a36081d. It'll be nice to have this fixed.
👍 theuni approved a pull request: "test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/pull/29907#pullrequestreview-2038175012)
ACK 976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b. Nice to have the serialization concept actually tested :)
(https://github.com/bitcoin/bitcoin/pull/29907#pullrequestreview-2038175012)
ACK 976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b. Nice to have the serialization concept actually tested :)
💬 instagibbs commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093051660)
@GregTonoski `MAX_SCRIPT_ELEMENT_SIZE` is a constant that describes a consensus rule that has been around since satoshi-era where nothing can be pushed to the stack that is larger than 520 bytes. There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093051660)
@GregTonoski `MAX_SCRIPT_ELEMENT_SIZE` is a constant that describes a consensus rule that has been around since satoshi-era where nothing can be pushed to the stack that is larger than 520 bytes. There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
💬 maflcko commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093052835)
> I can't repro the `-Wmaybe-uninitialized` issue.
It should repro in the CI env, at least. Or you can grep the CI log: https://cirrus-ci.com/task/6169505322237952?logs=ci#L3768
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093052835)
> I can't repro the `-Wmaybe-uninitialized` issue.
It should repro in the CI env, at least. Or you can grep the CI log: https://cirrus-ci.com/task/6169505322237952?logs=ci#L3768
💬 maflcko commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093058899)
Would it be easy to add mlc (or something like it) to the lint CI task? (Just as an idea for a follow-up, not for here)
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093058899)
Would it be easy to add mlc (or something like it) to the lint CI task? (Just as an idea for a follow-up, not for here)
💬 GregTonoski commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093065595)
> There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Are they "knock-on effects" or are the effects intentional?
>
> Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
Is the suggested name the best option? Why not another constant with another name, perhaps?
Is use case of OP_
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093065595)
> There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Are they "knock-on effects" or are the effects intentional?
>
> Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
Is the suggested name the best option? Why not another constant with another name, perhaps?
Is use case of OP_
💬 theuni commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093071416)
utACK with or without @hebasto's additional changes.
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093071416)
utACK with or without @hebasto's additional changes.
👍 ryanofsky approved a pull request: "refactor, test: Always initialize pointer"
(https://github.com/bitcoin/bitcoin/pull/30026#pullrequestreview-2038202995)
Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b
(https://github.com/bitcoin/bitcoin/pull/30026#pullrequestreview-2038202995)
Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b
💬 instagibbs commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093072872)
> Are they "knock-on effects" or are the effects intentional?
Doesn't matter for this PR, frankly. We need to be descriptive about the consensus bits in the code. I linked some historical background for your edification.
> Is the suggested name the best option? Why not another constant with another name, perhaps?
Probaby, because it's the constant that has been used for over a decade to describe a consensus-critical constant.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093072872)
> Are they "knock-on effects" or are the effects intentional?
Doesn't matter for this PR, frankly. We need to be descriptive about the consensus bits in the code. I linked some historical background for your edification.
> Is the suggested name the best option? Why not another constant with another name, perhaps?
Probaby, because it's the constant that has been used for over a decade to describe a consensus-critical constant.
🚀 ryanofsky merged a pull request: "refactor, test: Always initialize pointer"
(https://github.com/bitcoin/bitcoin/pull/30026)
(https://github.com/bitcoin/bitcoin/pull/30026)
💬 sipa commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093078874)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
> Is the suggested name the best option? Why not another constant with another name, perhaps?
The constant with this exact name and value already exists, it was introduced [11 years ago](https://github.com/bitcoin/bitcoin/pull/2188). There are just a few places in comments where the raw value 520 is used instead so far. This PR fixes that.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093078874)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
> Is the suggested name the best option? Why not another constant with another name, perhaps?
The constant with this exact name and value already exists, it was introduced [11 years ago](https://github.com/bitcoin/bitcoin/pull/2188). There are just a few places in comments where the raw value 520 is used instead so far. This PR fixes that.
💬 ryanofsky commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127)
Code review 8e394d1d3b6ead130515222f5b34d509fff200a8
I don't think it is good to change these to absolute links. When I open these files in emacs I can click the relative links and see the load the target files. If I change the links to be absolute, it tries to open non-existent paths on my root file system.
I imagine emacs is not the only tool which behaves this way. Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127)
Code review 8e394d1d3b6ead130515222f5b34d509fff200a8
I don't think it is good to change these to absolute links. When I open these files in emacs I can click the relative links and see the load the target files. If I change the links to be absolute, it tries to open non-existent paths on my root file system.
I imagine emacs is not the only tool which behaves this way. Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?