π ryanofsky merged a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603)
(https://github.com/bitcoin/bitcoin/pull/31603)
π¬ ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733092556)
@DrahtBot I've rebased and force-pushed 2 hours ago, but the update is yet to reflect here see https://github.com/bitcoin/bitcoin/compare/master...ismaelsadeeq:bitcoin:05-2023-ignore-transactions-with-parents
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733092556)
@DrahtBot I've rebased and force-pushed 2 hours ago, but the update is yet to reflect here see https://github.com/bitcoin/bitcoin/compare/master...ismaelsadeeq:bitcoin:05-2023-ignore-transactions-with-parents
π Chand-ra opened a pull request: "test: replace assert with assert_equal and assert_greater_than"
(https://github.com/bitcoin/bitcoin/pull/32091)
In `test/functional/interface_usdt_net.py`, `assert_equal` is already used to check for equality between objects. Replace `assert.*==` with `assert_equal` and `assert.*>` with `assert_greater_than` to further easify debugging.
(https://github.com/bitcoin/bitcoin/pull/32091)
In `test/functional/interface_usdt_net.py`, `assert_equal` is already used to check for equality between objects. Replace `assert.*==` with `assert_equal` and `assert.*>` with `assert_greater_than` to further easify debugging.
π¬ maflcko commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733111126)
This is a GitHub bug, but they are not going to fix it. The only way to fix it is by doing another rebase (or for a GitHub staff to manually clean it). See https://github.com/maflcko/DrahtBot/issues/43#issuecomment-2653314333
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733111126)
This is a GitHub bug, but they are not going to fix it. The only way to fix it is by doing another rebase (or for a GitHub staff to manually clean it). See https://github.com/maflcko/DrahtBot/issues/43#issuecomment-2653314333
π¬ maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733115158)
lgtm ACK 93fdaf1ca00de0bc46eb2455732582c34cef58b5
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733115158)
lgtm ACK 93fdaf1ca00de0bc46eb2455732582c34cef58b5
π¬ maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2000980971)
actually, this looks wrong. Let's hope the test/CI fails.
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2000980971)
actually, this looks wrong. Let's hope the test/CI fails.
π¬ fanquake commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733129056)
> Sure. Leaving it for follow ups.
Any particular reason? This has no ACKs, and it seems like removing the Wine workarounds should be a part of the rest of the Wine removal?
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733129056)
> Sure. Leaving it for follow ups.
Any particular reason? This has no ACKs, and it seems like removing the Wine workarounds should be a part of the rest of the Wine removal?
π¬ hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733143518)
> > Wine is not a supported platform for Bitcoin Core.
>
> To be fair, being able to test under Wine can be useful, if you're building/developing/testing something for windows but don't have a native windows machine available immediately. Sure it's possible to e.g. create windows server VMs at amazon EC but the extra cost and hassle is only warranted for final testing. But also agree that Wine support is not something that's worth putting a lot of effort in maintaining. But also wouldn't want
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733143518)
> > Wine is not a supported platform for Bitcoin Core.
>
> To be fair, being able to test under Wine can be useful, if you're building/developing/testing something for windows but don't have a native windows machine available immediately. Sure it's possible to e.g. create windows server VMs at amazon EC but the extra cost and hassle is only warranted for final testing. But also agree that Wine support is not something that's worth putting a lot of effort in maintaining. But also wouldn't want
...
π¬ brunoerg commented on issue "Failure to run Fuzz tests when running with corpus":
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733144415)
I could reproduce it following the same steps on MacOS 14.3
```
FUZZ=process_message build_fuzz/bin/fuzz qa-assets/fuzz_corpora/process_message/
fuzz(56832,0x1e31a5c40) malloc: nano zone abandoned due to inability to reserve vm space.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2192399851
INFO: Loaded 1 modules (1252322 inline 8-bit counters): 1252322 [0x104c38000, 0x104d69be2),
INFO: Loaded 1 PC tables (1252322 PCs): 1252322 [0x104d69be8,0x106085a08),
================
...
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733144415)
I could reproduce it following the same steps on MacOS 14.3
```
FUZZ=process_message build_fuzz/bin/fuzz qa-assets/fuzz_corpora/process_message/
fuzz(56832,0x1e31a5c40) malloc: nano zone abandoned due to inability to reserve vm space.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2192399851
INFO: Loaded 1 modules (1252322 inline 8-bit counters): 1252322 [0x104c38000, 0x104d69be2),
INFO: Loaded 1 PC tables (1252322 PCs): 1252322 [0x104d69be8,0x106085a08),
================
...
π¬ ismaelsadeeq commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2733158691)
It will be verbose to update all instances to use sat/vB instead of BTC/kvB in one go. Moreover, I imagine this will be a breaking change.
I also understand @maflcko's concern that having both units in the code makes things somewhat ambiguous for clients.
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2733158691)
It will be verbose to update all instances to use sat/vB instead of BTC/kvB in one go. Moreover, I imagine this will be a breaking change.
I also understand @maflcko's concern that having both units in the code makes things somewhat ambiguous for clients.
π¬ maflcko commented on issue "Failure to run Fuzz tests when running with corpus":
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733160346)
I presume the `libfuzzer-nosan` preset works fine?
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733160346)
I presume the `libfuzzer-nosan` preset works fine?
π¬ Prabhat1308 commented on issue "Failure to run Fuzz tests when running with corpus":
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733185782)
> I presume the `libfuzzer-nosan` preset works fine?
Works fine. I do get these warning though on the start of the run .
```
WARNING: Failed to find function "__sanitizer_acquire_crash_state". Reason dlsym(RTLD_DEFAULT, __sanitizer_acquire_crash_state): symbol not found.
WARNING: Failed to find function "__sanitizer_print_stack_trace". Reason dlsym(RTLD_DEFAULT, __sanitizer_print_stack_trace): symbol not found.
WARNING: Failed to find function "__sanitizer_set_death_callback". Reason dlsym(RTLD
...
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2733185782)
> I presume the `libfuzzer-nosan` preset works fine?
Works fine. I do get these warning though on the start of the run .
```
WARNING: Failed to find function "__sanitizer_acquire_crash_state". Reason dlsym(RTLD_DEFAULT, __sanitizer_acquire_crash_state): symbol not found.
WARNING: Failed to find function "__sanitizer_print_stack_trace". Reason dlsym(RTLD_DEFAULT, __sanitizer_print_stack_trace): symbol not found.
WARNING: Failed to find function "__sanitizer_set_death_callback". Reason dlsym(RTLD
...
π¬ polespinasa commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2733192225)
We can look at what changes are needed, update the ones that don't break anything (e.g. rpc that simply give information but don't take a feerate as an argument). And the others can be put together in a PR, if they are not too many or too critical we can warn during a couple of releases with βdeprecatedβ and at some point update to change the units.
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2733192225)
We can look at what changes are needed, update the ones that don't break anything (e.g. rpc that simply give information but don't take a feerate as an argument). And the others can be put together in a PR, if they are not too many or too critical we can warn during a couple of releases with βdeprecatedβ and at some point update to change the units.
π¬ brunoerg commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2733198404)
> > Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
> > ```shell
> > ccache -C && git clean -fxd && git reset --hard
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Guess we can't rely on cache invalidation here.
>
> I'd appreciate it if you could provide steps to reproduce the scenario where
...
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2733198404)
> > Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
> > ```shell
> > ccache -C && git clean -fxd && git reset --hard
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Guess we can't rely on cache invalidation here.
>
> I'd appreciate it if you could provide steps to reproduce the scenario where
...
π¬ maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r2001035909)
thx, done
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r2001035909)
thx, done
π¬ maflcko commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2733223668)
Another instance where a stale cmake cache caused issues after a brew? update: https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2733223668)
Another instance where a stale cmake cache caused issues after a brew? update: https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053
π¬ laanwj commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733226368)
> From my experience, it can be a bit annoying when working on Windows-specific code one also have to accommodate Wine-specific code in tests.
Yes, ideally incompatibilities with real windows would be fixed on the Wine side, the whole point of Wine is to behave like windows. It's absurd to have to carry a lot of workarounds just for Wine.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733226368)
> From my experience, it can be a bit annoying when working on Windows-specific code one also have to accommodate Wine-specific code in tests.
Yes, ideally incompatibilities with real windows would be fixed on the Wine side, the whole point of Wine is to behave like windows. It's absurd to have to carry a lot of workarounds just for Wine.
π¬ maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733252346)
As the test failures under wine happen intermittently, I expect it would be hard to debug and fix it upstream? Maybe someone can reproduce it in `rr`?
In any case, I don't mind if people maintain Wine compatibility, or even add it back to the normal CI, once Wine is fixed. However, until then, maintaining Wine compatibility is something to be done in a "nightly CI", with only fixes provided to this repo.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2733252346)
As the test failures under wine happen intermittently, I expect it would be hard to debug and fix it upstream? Maybe someone can reproduce it in `rr`?
In any case, I don't mind if people maintain Wine compatibility, or even add it back to the normal CI, once Wine is fixed. However, until then, maintaining Wine compatibility is something to be done in a "nightly CI", with only fixes provided to this repo.
π¬ maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001077998)
https://github.com/bitcoin/bitcoin/actions/runs/13923574432/job/38962671641?pr=32091#step:6:7169
failed
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001077998)
https://github.com/bitcoin/bitcoin/actions/runs/13923574432/job/38962671641?pr=32091#step:6:7169
failed
π¬ Chand-ra commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001094779)
I ran the entire test suite from my `build` directory and the modified test passed just fine there. Any idea on what might cause the discrepancy?
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001094779)
I ran the entire test suite from my `build` directory and the modified test passed just fine there. Any idea on what might cause the discrepancy?