π¬ MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1803471329)
In the dart library coinlib (https://pub.dev/packages/coinlib) I followed the same approach of encoding everything in HEX with integers encoded as LE. However, 0 and -1 are encoded as 0 and -1. See the code here: https://github.com/peercoin/coinlib/blob/master/coinlib/lib/src/scripts/operations.dart
I did this to avoid changing the ASM too much. However, I'm happy to move towards using `0x` for hex and presenting decimal for smaller data of up-to 32bits if this is introduced in Bitcoin. This
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1803471329)
In the dart library coinlib (https://pub.dev/packages/coinlib) I followed the same approach of encoding everything in HEX with integers encoded as LE. However, 0 and -1 are encoded as 0 and -1. See the code here: https://github.com/peercoin/coinlib/blob/master/coinlib/lib/src/scripts/operations.dart
I did this to avoid changing the ASM too much. However, I'm happy to move towards using `0x` for hex and presenting decimal for smaller data of up-to 32bits if this is introduced in Bitcoin. This
...
π¬ maflcko commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387766074)
Maybe submit this in a separate pull for review?
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1387766074)
Maybe submit this in a separate pull for review?
π¬ dergoegge commented on pull request "fuzz: Minor improvements to tx_package_eval target":
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1387708805)
Maybe put this in `test/util/script.h`?
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1387708805)
Maybe put this in `test/util/script.h`?
π fanquake locked a pull request: "[doc]: uppercase title for "Assumeutxo""
(https://github.com/bitcoin/bitcoin/pull/28827)
uppercase title from `# assumeutxo` to `# Assumeutxo`
(https://github.com/bitcoin/bitcoin/pull/28827)
uppercase title from `# assumeutxo` to `# Assumeutxo`
π¬ dergoegge commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803538946)
> I can add steps to reproduce, if you want.
That would be great.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803538946)
> I can add steps to reproduce, if you want.
That would be great.
π¬ fanquake commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1803550311)
Another thing to note, is that the code being patched here, has been deleted entirely upstream: https://github.com/qt/qtbase/commit/329db8b64f17c8ef013c586cea1f1c5b49c4a4b7.
> macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
> Availability.h from the platform SDK should take care of this these days.
So this points to there still being an underlying issue, and workaround will "break" if/when we start using newer versions of Qt.
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1803550311)
Another thing to note, is that the code being patched here, has been deleted entirely upstream: https://github.com/qt/qtbase/commit/329db8b64f17c8ef013c586cea1f1c5b49c4a4b7.
> macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
> Availability.h from the platform SDK should take care of this these days.
So this points to there still being an underlying issue, and workaround will "break" if/when we start using newer versions of Qt.
π fanquake merged a pull request: "test: Add missing wait for version to be sent in add_outbound_p2p_connection"
(https://github.com/bitcoin/bitcoin/pull/28822)
(https://github.com/bitcoin/bitcoin/pull/28822)
π¬ maflcko commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1803592254)
> I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call Ban with an invalid subnet/netaddr.
Would be good to explain this a bit more, to make sure this is not a bug.
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1803592254)
> I did some experiments and I decided to keep the lookup functions. Without it, I still get discrepances even avoiding the comparison when we call Ban with an invalid subnet/netaddr.
Would be good to explain this a bit more, to make sure this is not a bug.
π¬ fanquake commented on pull request "doc: update docs for `CHECK_ATOMIC` macro":
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1803597897)
> Could this moment be now?
Feel free to open an issue for broader discussion. Given the overhead of supporting it is low, I don't see a pressing reason to do so. There is also a difference between us officially dropping support, and purposefully breaking the ability to compile for certain targets.
(https://github.com/bitcoin/bitcoin/pull/28777#issuecomment-1803597897)
> Could this moment be now?
Feel free to open an issue for broader discussion. Given the overhead of supporting it is low, I don't see a pressing reason to do so. There is also a difference between us officially dropping support, and purposefully breaking the ability to compile for certain targets.
π fanquake opened a pull request: "ci: win64 task does use boost:process"
(https://github.com/bitcoin/bitcoin/pull/28829)
It passes `--enable-external-signer`.
(https://github.com/bitcoin/bitcoin/pull/28829)
It passes `--enable-external-signer`.
π¬ maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803615609)
Steps to reproduce flamegraph:
* Compile fuzz tests normally (libFuzzer recommended)
* `perf record`, for example via `FUZZ=mocked_descriptor_parse perf record -g --call-graph dwarf -F 1001 ./src/test/fuzz/fuzz -runs=2 /tmp/fuzz_input.dat` (`-runs` can be increased to discount an expensive global setup initialization, `--call-graph` and frequency can be modified depending on your system)
* Load the data as a flamegraph: `hotspot ./perf.data`
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803615609)
Steps to reproduce flamegraph:
* Compile fuzz tests normally (libFuzzer recommended)
* `perf record`, for example via `FUZZ=mocked_descriptor_parse perf record -g --call-graph dwarf -F 1001 ./src/test/fuzz/fuzz -runs=2 /tmp/fuzz_input.dat` (`-runs` can be increased to discount an expensive global setup initialization, `--call-graph` and frequency can be modified depending on your system)
* Load the data as a flamegraph: `hotspot ./perf.data`
π¬ maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803617915)
cc @darosior about `mocked_descriptor_parse`
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803617915)
cc @darosior about `mocked_descriptor_parse`
π¬ darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803618906)
Yeah i was looking into it. It's surprising we are spending so long inside the dup key check. Maybe we re-introduced the issue fixed by https://github.com/bitcoin/bitcoin/pull/25540.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1803618906)
Yeah i was looking into it. It's surprising we are spending so long inside the dup key check. Maybe we re-introduced the issue fixed by https://github.com/bitcoin/bitcoin/pull/25540.
π¬ maflcko commented on pull request "ci: win64 task does use boost:process":
(https://github.com/bitcoin/bitcoin/pull/28829#issuecomment-1803619984)
lgtm ACK 5f0bf2ef69a607433f58de3364dee10ad347f3e1
(https://github.com/bitcoin/bitcoin/pull/28829#issuecomment-1803619984)
lgtm ACK 5f0bf2ef69a607433f58de3364dee10ad347f3e1
π¬ stickies-v commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1387845814)
nit: I think this docstring should now be removed, I don't think the constructor requires a mempool.cs lock anymore?
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1387845814)
nit: I think this docstring should now be removed, I don't think the constructor requires a mempool.cs lock anymore?
π fanquake merged a pull request: "ci: win64 task does use boost:process"
(https://github.com/bitcoin/bitcoin/pull/28829)
(https://github.com/bitcoin/bitcoin/pull/28829)
π TheCharlatan opened a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830)
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
(https://github.com/bitcoin/bitcoin/pull/28830)
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
π¬ TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1803653460)
Rebased 1a589335617944b755235597aafc254e28e4acf9 -> 40e151697d3d1ed594364498d9e6220219d9b545 ([kernelInternalLib_4](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_4) -> [kernelInternalLib_5](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_4..kernelInternalLib_5))
* Fixed merge conflict.
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1803653460)
Rebased 1a589335617944b755235597aafc254e28e4acf9 -> 40e151697d3d1ed594364498d9e6220219d9b545 ([kernelInternalLib_4](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_4) -> [kernelInternalLib_5](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_4..kernelInternalLib_5))
* Fixed merge conflict.
π darosior approved a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-1722332060)
utACK fa520848da6d718a07368b42b1a44bd2515e6e5a
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-1722332060)
utACK fa520848da6d718a07368b42b1a44bd2515e6e5a
π¬ 0xB10C commented on pull request "doc: uppercase title for "Assumeutxoβ":
(https://github.com/bitcoin/bitcoin/pull/28828#issuecomment-1803679113)
I don't think this adds any value here.
(https://github.com/bitcoin/bitcoin/pull/28828#issuecomment-1803679113)
I don't think this adds any value here.