π dergoegge opened a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)
(https://github.com/bitcoin/bitcoin/pull/31221)
π¬ dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2457369883)
cc @maflcko couldn't reopen the other PR after force pushing
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2457369883)
cc @maflcko couldn't reopen the other PR after force pushing
π dergoegge converted_to_draft a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)
(https://github.com/bitcoin/bitcoin/pull/31221)
π¬ instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829485990)
will take language if I touch PR again
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829485990)
will take language if I touch PR again
π¬ Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829528839)
I think this was already the case before this PR? It seems that the currently implementation in solver.cpp doesn't handle `OP_0` correctly, while the python codebase does?
https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/test/functional/test_framework/script.py#L89
https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/script/solver.cpp#L61
This goes to the purpose of this PR, handle them consistently across codebases and re
...
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829528839)
I think this was already the case before this PR? It seems that the currently implementation in solver.cpp doesn't handle `OP_0` correctly, while the python codebase does?
https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/test/functional/test_framework/script.py#L89
https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/script/solver.cpp#L61
This goes to the purpose of this PR, handle them consistently across codebases and re
...
π¬ Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2457443510)
Thanks for looking at this @mzumsande !
> I didnβt see earlier versions of this PR, but Itβs not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?
Modified the title, hopefully its more clear.
> As mentioned above, `DecodeOP_N` / `EncodeOP_N` currently don't need to handle `OP_1NEGATE` in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.
...
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2457443510)
Thanks for looking at this @mzumsande !
> I didnβt see earlier versions of this PR, but Itβs not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?
Modified the title, hopefully its more clear.
> As mentioned above, `DecodeOP_N` / `EncodeOP_N` currently don't need to handle `OP_1NEGATE` in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.
...
π¬ Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829540702)
In f2f8796 I modified `IsSmallInteger()` to handle `OP_1NEGATE` _and_ `OP_0` correctly. Previously only the python codebase handled `OP_0` correctly.
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829540702)
In f2f8796 I modified `IsSmallInteger()` to handle `OP_1NEGATE` _and_ `OP_0` correctly. Previously only the python codebase handled `OP_0` correctly.
π¬ mzumsande commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2457459639)
> What about having P2P-seeds, an alternative to DNS-seeds, which are serving the P2P protocol and are used as addrfetch peers and they return only high-quality addresses from the crawler?
Yes, I think that could work.
Also, I wouldn't say that this particular downside I mentioned would be a blocker - even if it would take on average 1 minute instead of 20 seconds to find the initial 10 peers, that wouldn't be the end of the world, since it's just a one-time event for an empty addrman.
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2457459639)
> What about having P2P-seeds, an alternative to DNS-seeds, which are serving the P2P protocol and are used as addrfetch peers and they return only high-quality addresses from the crawler?
Yes, I think that could work.
Also, I wouldn't say that this particular downside I mentioned would be a blocker - even if it would take on average 1 minute instead of 20 seconds to find the initial 10 peers, that wouldn't be the end of the world, since it's just a one-time event for an empty addrman.
π¬ darosior commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2457482792)
Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2457482792)
Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.
π¬ mzumsande commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829569059)
I don't think that "correct" is the right term here, it's more about what these functions are intended to be used for. In this case, the comment `Test for "small positive integer"` indicates that the function wasn't designed to deal with `OP_0`, so if just the name could just rename it to `IsSmallPositiveInteger` to not introduce a change in behavior.
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829569059)
I don't think that "correct" is the right term here, it's more about what these functions are intended to be used for. In this case, the comment `Test for "small positive integer"` indicates that the function wasn't designed to deal with `OP_0`, so if just the name could just rename it to `IsSmallPositiveInteger` to not introduce a change in behavior.
π¬ dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2457529378)
Looks like using the matrix feature works for this
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2457529378)
Looks like using the matrix feature works for this
π dergoegge's pull request is ready for review: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)
(https://github.com/bitcoin/bitcoin/pull/31221)
π¬ marcofleon commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1829592727)
The upper target is the `nBits` value from the genesis block (0x1D00FFFF) and the lower one is the `nBits` from some mainnet block at a certain height (0x17058EBE). Using `SetCompact` on those values gives those two uint256 values. The work returned from the lower target multiplied by the longest possible chain (1600) in the test should be lower than `MinimumChainWork`.
I get 0x11fe50e0ab9cbcf864c0500 for the theoretical max amount of work a headers chain from this test could have. `MinimumC
...
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1829592727)
The upper target is the `nBits` value from the genesis block (0x1D00FFFF) and the lower one is the `nBits` from some mainnet block at a certain height (0x17058EBE). Using `SetCompact` on those values gives those two uint256 values. The work returned from the lower target multiplied by the longest possible chain (1600) in the test should be lower than `MinimumChainWork`.
I get 0x11fe50e0ab9cbcf864c0500 for the theoretical max amount of work a headers chain from this test could have. `MinimumC
...
π¬ marcofleon commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1829593835)
Done.
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1829593835)
Done.
π¬ fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2457548171)
I could remove another `sleep` import line after https://github.com/bitcoin/bitcoin/pull/30942 was merged which was what the linter complained about. Should be trivial to re-ACK: https://github.com/bitcoin/bitcoin/compare/352b8209aa5327f7d369e2acc4d87f9767389a6b..f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2457548171)
I could remove another `sleep` import line after https://github.com/bitcoin/bitcoin/pull/30942 was merged which was what the linter complained about. Should be trivial to re-ACK: https://github.com/bitcoin/bitcoin/compare/352b8209aa5327f7d369e2acc4d87f9767389a6b..f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96
π¬ fanquake commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2457600736)
Guix build:
```bash
67b7e95e0f1d45debad8a8cac4c2d349e3e01efcf62e0b109345896f296758d9 guix-build-788c1324f3d8/output/aarch64-linux-gnu/SHA256SUMS.part
cd3c67eaa5f6fadf0a73cdb1aa03563d720fc7fdae1b59aa495a0eda9d2695af guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu-debug.tar.gz
5c1eb4ff827336a9d113497459981e213844b87e232c33fc00317ce20302f761 guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu.tar.gz
dd78923f8fb7410a
...
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2457600736)
Guix build:
```bash
67b7e95e0f1d45debad8a8cac4c2d349e3e01efcf62e0b109345896f296758d9 guix-build-788c1324f3d8/output/aarch64-linux-gnu/SHA256SUMS.part
cd3c67eaa5f6fadf0a73cdb1aa03563d720fc7fdae1b59aa495a0eda9d2695af guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu-debug.tar.gz
5c1eb4ff827336a9d113497459981e213844b87e232c33fc00317ce20302f761 guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu.tar.gz
dd78923f8fb7410a
...
π fanquake approved a pull request: "depends, doc: List packages required to build `qt` package separately"
(https://github.com/bitcoin/bitcoin/pull/31192#pullrequestreview-2416094490)
ACK 4747f030956d6876ec7cef4e2500a8579bc82146
(https://github.com/bitcoin/bitcoin/pull/31192#pullrequestreview-2416094490)
ACK 4747f030956d6876ec7cef4e2500a8579bc82146
π fanquake merged a pull request: "depends, doc: List packages required to build `qt` package separately"
(https://github.com/bitcoin/bitcoin/pull/31192)
(https://github.com/bitcoin/bitcoin/pull/31192)
π¬ fanquake commented on pull request "doc: Fix word order in developer-notes.md":
(https://github.com/bitcoin/bitcoin/pull/31220#issuecomment-2457604363)
ACK 44939e5de1b37ccd85ef31268219c5866bd3ee3f
(https://github.com/bitcoin/bitcoin/pull/31220#issuecomment-2457604363)
ACK 44939e5de1b37ccd85ef31268219c5866bd3ee3f
π fanquake merged a pull request: "doc: Fix word order in developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31220)
(https://github.com/bitcoin/bitcoin/pull/31220)