💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648901099)
Thanks for the review Eunovo. Feedback tackled.
> While testing, I noticed that trying to abandon a tx in mempool wasn't covered in the functional tests
That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648901099)
Thanks for the review Eunovo. Feedback tackled.
> While testing, I noticed that trying to abandon a tx in mempool wasn't covered in the functional tests
That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.
👋 fjahr's pull request is ready for review: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803)
(https://github.com/bitcoin/bitcoin/pull/31803)
💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-2648910685)
https://github.com/bitcoin/bitcoin/pull/31384 was merged, so this is rebased and ready for review.
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-2648910685)
https://github.com/bitcoin/bitcoin/pull/31384 was merged, so this is rebased and ready for review.
💬 Eunovo commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648918983)
> That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.
It'd be a really tiny PR. I think you can just add it but I'm happy to open a new one if needed.
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648918983)
> That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.
It'd be a really tiny PR. I think you can just add it but I'm happy to open a new one if needed.
💬 Eunovo commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648923341)
ReACK https://github.com/bitcoin/bitcoin/pull/31794/commits/409241db5dca0b23f5c7714f99be52411fc5541e
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2648923341)
ReACK https://github.com/bitcoin/bitcoin/pull/31794/commits/409241db5dca0b23f5c7714f99be52411fc5541e
📝 mzumsande opened a pull request: "test: add missing sync to p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/31837)
If the outbound peer hasn't processed the inv before the mocktime bump, it won't be preferred after the timeout, failing the test . Therefore, add a sync, just like there is one in the previous `send_message` calls.
Fixes #31833
(https://github.com/bitcoin/bitcoin/pull/31837)
If the outbound peer hasn't processed the inv before the mocktime bump, it won't be preferred after the timeout, failing the test . Therefore, add a sync, just like there is one in the previous `send_message` calls.
Fixes #31833
💬 mzumsande commented on issue "ci: intermittent p2p_tx_download.py timeout (test_preferred_tiebreaker_inv)":
(https://github.com/bitcoin/bitcoin/issues/31833#issuecomment-2648924987)
Looks like it's just a missing sync, see #31837.
(https://github.com/bitcoin/bitcoin/issues/31833#issuecomment-2648924987)
Looks like it's just a missing sync, see #31837.
💬 mzumsande commented on issue "qa: Functional tests are intrinsic vulnerable to timeouts":
(https://github.com/bitcoin/bitcoin/issues/19732#issuecomment-2648930123)
> Is [#31833](https://github.com/bitcoin/bitcoin/issues/31833) a symptom of this?
I don't think so, just a missing sync there.
(https://github.com/bitcoin/bitcoin/issues/19732#issuecomment-2648930123)
> Is [#31833](https://github.com/bitcoin/bitcoin/issues/31833) a symptom of this?
I don't think so, just a missing sync there.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949710626)
Gone.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949710626)
Gone.
💬 l0rinc commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2648966500)
I ran it on an `Ubuntu 24.04` Hetzner server as well and basically got the same result (that `0100007f` is not equal to `7f000001`):
```bash
# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
```
1/3 - rpc_bind.py --ipv4 failed, Duration: 2 s
...
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2648966500)
I ran it on an `Ubuntu 24.04` Hetzner server as well and basically got the same result (that `0100007f` is not equal to `7f000001`):
```bash
# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
```
1/3 - rpc_bind.py --ipv4 failed, Duration: 2 s
...
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949719629)
Fuzzing is still random, it's just biased towards increasing coverage. It's not specifically intended to trigger bugs, it just aims to maximize coverage (code coverage, branch coverage, and "values being compared" coverage). And typically, this is exactly what you want in correctness tests too, as it may result in far more edge/special cases being exercised than uniformly random inputs may trigger.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949719629)
Fuzzing is still random, it's just biased towards increasing coverage. It's not specifically intended to trigger bugs, it just aims to maximize coverage (code coverage, branch coverage, and "values being compared" coverage). And typically, this is exactly what you want in correctness tests too, as it may result in far more edge/special cases being exercised than uniformly random inputs may trigger.
💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2649002546)
Nice. We do have something similar, although it could likely be refined more. You can see it [here](https://github.com/marcofleon/coverage/tree/master/deterministic-coverage). It should show all the places where hit count between two coverage reports differs.
I generated the json files with
```
llvm-cov export ./fuzzbuild/src/test/fuzz/fuzz -instr-profile=coverage.profdata > coverage.json
```
and then run it like this
```
python3 compare_coverage.py p2p_headers_presync.json p2p_headers
...
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2649002546)
Nice. We do have something similar, although it could likely be refined more. You can see it [here](https://github.com/marcofleon/coverage/tree/master/deterministic-coverage). It should show all the places where hit count between two coverage reports differs.
I generated the json files with
```
llvm-cov export ./fuzzbuild/src/test/fuzz/fuzz -instr-profile=coverage.profdata > coverage.json
```
and then run it like this
```
python3 compare_coverage.py p2p_headers_presync.json p2p_headers
...
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1949743176)
`ischange` does not actually look at the derivation path. Instead it looks at some heuristics which result in derived-but-not-yet-requested external addresses being identified as change.
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1949743176)
`ischange` does not actually look at the derivation path. Instead it looks at some heuristics which result in derived-but-not-yet-requested external addresses being identified as change.
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1949743508)
Going to leave this as is.
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1949743508)
Going to leave this as is.
💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949745452)
It seems to me that `std::floor` and `std::ceil` already have overloads for `long double`, which we could use instead to appease the CI gods:
> feefrac.cpp:144:44: error: ‘floorl’ is not a member of ‘std’; did you mean ‘floor’?
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949745452)
It seems to me that `std::floor` and `std::ceil` already have overloads for `long double`, which we could use instead to appease the CI gods:
> feefrac.cpp:144:44: error: ‘floorl’ is not a member of ‘std’; did you mean ‘floor’?
💬 pseudoramdom commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649015969)
Hi @ryanofsky! I'm new to this and I'm looking to test the feature on macOS. Couple of questions -
1. Can I use the branch as is or would I need to rebase on `master` since I see some related changes like #31740, #31552 landed on `master`.
2. Do I need to install `libmultiprocess` from source or is `MULTIPROCESS=1` needed? I see some related work in #31741 . Would I benefit from that?
I tried building on this branch rebased on `master` and I got the following errors while building.
...
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649015969)
Hi @ryanofsky! I'm new to this and I'm looking to test the feature on macOS. Couple of questions -
1. Can I use the branch as is or would I need to rebase on `master` since I see some related changes like #31740, #31552 landed on `master`.
2. Do I need to install `libmultiprocess` from source or is `MULTIPROCESS=1` needed? I see some related work in #31741 . Would I benefit from that?
I tried building on this branch rebased on `master` and I got the following errors while building.
...
👍 l0rinc approved a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2606987771)
ACK cf00540f54302e37f8af08f15fce16cbcab0f8c9
Added approximately floating point comparisons to fuzz tests and removed the lingering `[[maybe_unused]]` in `feefrac_tests.cpp`
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2606987771)
ACK cf00540f54302e37f8af08f15fce16cbcab0f8c9
Added approximately floating point comparisons to fuzz tests and removed the lingering `[[maybe_unused]]` in `feefrac_tests.cpp`
💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949777887)
I was wondering if we could measure [ULP tolerance](https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition) here instead to better control the step sizes - or if that would be too much effort in this context.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949777887)
I was wondering if we could measure [ULP tolerance](https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition) here instead to better control the step sizes - or if that would be too much effort in this context.
💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649091209)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649015969
> Can I use the branch as is or would I need to rebase on `master`
Hi @pseudoramdom, definitely it should be easiest to just check out this branch and use it as is. The errors that you are seeing with the branch rebased happen due to https://github.com/chaincodelabs/libmultiprocess/pull/135, and require some code changes here to be compatible. I should be able to update this branch in the next day or two in case you
...
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649091209)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2649015969
> Can I use the branch as is or would I need to rebase on `master`
Hi @pseudoramdom, definitely it should be easiest to just check out this branch and use it as is. The errors that you are seeing with the branch rebased happen due to https://github.com/chaincodelabs/libmultiprocess/pull/135, and require some code changes here to be compatible. I should be able to update this branch in the next day or two in case you
...
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949791787)
Honestly, I think the code I included here already is more complicated than what I think was worth writing. But it's just test code, and it's already written, and it works.
Its added value is adding confidence for anyone looking at the test who has difficulty following, the exact `arith_uint256`-based test code, or otherwise doubts its correctness. The exact precision used here won't make much difference I think - it should just be enough so someone can a single glance say "ok, if this test d
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949791787)
Honestly, I think the code I included here already is more complicated than what I think was worth writing. But it's just test code, and it's already written, and it works.
Its added value is adding confidence for anyone looking at the test who has difficulty following, the exact `arith_uint256`-based test code, or otherwise doubts its correctness. The exact precision used here won't make much difference I think - it should just be enough so someone can a single glance say "ok, if this test d
...