💬 mzumsande commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446)
The init value should be `CAmount{0}`, not `0` which would be converted to an `int`, see [std::accumulate](https://en.cppreference.com/w/cpp/algorithm/accumulate) under "Common mistakes". This caused the unexpected fuzzer behavior I saw, because the `int` could overflow.
Also, for `ancestor_package_size`, the init value should be `int64_t{0}`;
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446)
The init value should be `CAmount{0}`, not `0` which would be converted to an `int`, see [std::accumulate](https://en.cppreference.com/w/cpp/algorithm/accumulate) under "Common mistakes". This caused the unexpected fuzzer behavior I saw, because the `int` could overflow.
Also, for `ancestor_package_size`, the init value should be `int64_t{0}`;
💬 mzumsande commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136146262)
I commented below for this. After fixing this, the fuzzer doesn't crash anymore (at least not immediately, haven't run it for long).
I wonder if it might make sense to adjust the fuzz test permanently similar to my branch above and assert `sum(CalculateBumpFees) >= CalculateTotalBumpFees()`, if that is really the expectation - in general, I like it if fuzz tests contain asserts test actual non-trivial high-level expectations we have for the code, and don't just run over the code "blindly".
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136146262)
I commented below for this. After fixing this, the fuzzer doesn't crash anymore (at least not immediately, haven't run it for long).
I wonder if it might make sense to adjust the fuzz test permanently similar to my branch above and assert `sum(CalculateBumpFees) >= CalculateTotalBumpFees()`, if that is really the expectation - in general, I like it if fuzz tests contain asserts test actual non-trivial high-level expectations we have for the code, and don't just run over the code "blindly".
💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468791319)
@desirepl ok got it, I can reproduce. I think this is only an issue with the GUI. I also noticed one or two other little oddities around config file and will open a PR soon.
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468791319)
@desirepl ok got it, I can reproduce. I think this is only an issue with the GUI. I also noticed one or two other little oddities around config file and will open a PR soon.
⚠️ fanquake opened an issue: "feature_config_args.py failure under `--valgrind`"
(https://github.com/bitcoin/bitcoin/issues/27259)
This doesn't fail inside the Valgrind CI system, but seems to consistently fail in a (non-timeout) fashion when running the functional tests under `--valgrind`. Done on Ubuntu 22.04, with Vagrind `valgrind-3.21.0.GIT`. master at 460e394625fab2942748aaeec9be31f460f91c58.
```bash
254/262 - feature_config_args.py failed, Duration: 54 s
stdout:
2023-03-14T20:40:35.820000Z TestFramework (INFO): PRNG seed is: 3139282899219230712
2023-03-14T20:40:35.821000Z TestFramework (INFO): Initializing tes
...
(https://github.com/bitcoin/bitcoin/issues/27259)
This doesn't fail inside the Valgrind CI system, but seems to consistently fail in a (non-timeout) fashion when running the functional tests under `--valgrind`. Done on Ubuntu 22.04, with Vagrind `valgrind-3.21.0.GIT`. master at 460e394625fab2942748aaeec9be31f460f91c58.
```bash
254/262 - feature_config_args.py failed, Duration: 54 s
stdout:
2023-03-14T20:40:35.820000Z TestFramework (INFO): PRNG seed is: 3139282899219230712
2023-03-14T20:40:35.821000Z TestFramework (INFO): Initializing tes
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136182559)
I think I need to touch five or six different places for this. Is that worth it?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136182559)
I think I need to touch five or six different places for this. Is that worth it?
💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1136210879)
This is just being moved here and I don't think this can overflow in practice, but the init value should be `int64_t{0}`, so maybe it would make sense to fix this somewhere within this PR. (searched the rest of the codebase after https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446).
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1136210879)
This is just being moved here and I don't think this can overflow in practice, but the init value should be `int64_t{0}`, so maybe it would make sense to fix this somewhere within this PR. (searched the rest of the codebase after https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136136446).
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1468854086)
> What do you think?
As @stickies-v explained to me outside this PR: "... by moving the uri parsing to http_request_cb we're failing super early, and even when the query parameter would never be called by the endpoint... ", this makes more sense.
I'm working on this approach which I think it's better as well.
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1468854086)
> What do you think?
As @stickies-v explained to me outside this PR: "... by moving the uri parsing to http_request_cb we're failing super early, and even when the query parameter would never be called by the endpoint... ", this makes more sense.
I'm working on this approach which I think it's better as well.
💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468925984)
@pinheadmz bitcoin-cli also ignores .conf's datadir, so not only GUI
maybe bitcoind too, didn't checked it, because spent too much time for -qt to understand why it's not working as must be
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468925984)
@pinheadmz bitcoin-cli also ignores .conf's datadir, so not only GUI
maybe bitcoind too, didn't checked it, because spent too much time for -qt to understand why it's not working as must be
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136214058)
I’ve updated the comment to mention the 500 entry limit and the resulting empty vector.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136214058)
I’ve updated the comment to mention the 500 entry limit and the resulting empty vector.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136311603)
Thanks, I adopted your suggestion.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136311603)
Thanks, I adopted your suggestion.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136215648)
Good catch. I’ve removed the line and amended the comment to clarify that the transaction is part of the descendants.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136215648)
Good catch. I’ve removed the line and amended the comment to clarify that the transaction is part of the descendants.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136294354)
Thanks I took your suggestion with slightly more elaborate variable names.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136294354)
Thanks I took your suggestion with slightly more elaborate variable names.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136217858)
Thanks, I’ve adopted your suggestion and moved `!mempool.exists` first.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136217858)
Thanks, I’ve adopted your suggestion and moved `!mempool.exists` first.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136319884)
Thanks, this improves the fuzzer. I’ve adopted your suggested change.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136319884)
Thanks, this improves the fuzzer. I’ve adopted your suggested change.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136299879)
Uff, great catch, thanks!
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136299879)
Uff, great catch, thanks!
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136330617)
I see how this might be a performance improvement, but I don’t think this is going to be heavy enough to warrant this level of rewrite at this stage in the PR. Perhaps this could be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1136330617)
I see how this might be a performance improvement, but I don’t think this is going to be heavy enough to warrant this level of rewrite at this stage in the PR. Perhaps this could be done in a follow-up.
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1469045147)
force-pushed: Added `self.whitelist_peers` in `test_framework` to avoid having to add `-whitelist` flag everywhere.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1469045147)
force-pushed: Added `self.whitelist_peers` in `test_framework` to avoid having to add `-whitelist` flag everywhere.
👍 ajtowns approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
ACK 600ab02bf58e073a93936438a7e884b3a7110f1c
(https://github.com/bitcoin/bitcoin/pull/26177)
ACK 600ab02bf58e073a93936438a7e884b3a7110f1c
💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136502150)
Yes, I have implemented this both for `walletlock` and `walletpassphrasechange` to ensure that this error is raised in both. However, it is important to note that this test now tests something slightly different than what it did previously. Previously this checked that the wallet was still able to complete a full rescan even if an attempt was made to relock the wallet (because of the timeout in `walletpassphrase`). This new test does not test this because other than using the `walletpassphrase`
...
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136502150)
Yes, I have implemented this both for `walletlock` and `walletpassphrasechange` to ensure that this error is raised in both. However, it is important to note that this test now tests something slightly different than what it did previously. Previously this checked that the wallet was still able to complete a full rescan even if an attempt was made to relock the wallet (because of the timeout in `walletpassphrase`). This new test does not test this because other than using the `walletpassphrase`
...
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1469360153)
I'm not sure it has any influence on this pr, but did you consider doing something similar for block-relay-only peers?
Something like: connect to one generic block-relay-only peer, plus an additional block-relay-only peer for every network we support? I think that should only improve things, including during IBD. It wouldn't do anything to improve tx propagation of course, so wouldn't make this PR redundant.
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1469360153)
I'm not sure it has any influence on this pr, but did you consider doing something similar for block-relay-only peers?
Something like: connect to one generic block-relay-only peer, plus an additional block-relay-only peer for every network we support? I think that should only improve things, including during IBD. It wouldn't do anything to improve tx propagation of course, so wouldn't make this PR redundant.