💬 laanwj commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951061433)
Eh yes, that would be better.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951061433)
Eh yes, that would be better.
💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951069695)
Yes, Updated.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951069695)
Yes, Updated.
💬 sipa commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651173341)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. I have not tested this.
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651173341)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. I have not tested this.
💬 mzumsande commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951078538)
didn't get to it before merge.
>not many remaining cases
well, I count ~130 still, so definitely some churn involved changing most of these. Still, it would make sense to me.
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951078538)
didn't get to it before merge.
>not many remaining cases
well, I count ~130 still, so definitely some churn involved changing most of these. Still, it would make sense to me.
💬 laanwj commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651183942)
re-ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i could test it on amazon graviton again like i did the original PR, but only the good-weather case, that won't reproduce the edge case that led to this.
Would be good if @giahuy98 (who opened #31817) could test it on the specific target hardware.
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651183942)
re-ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i could test it on amazon graviton again like i did the original PR, but only the good-weather case, that won't reproduce the edge case that led to this.
Would be good if @giahuy98 (who opened #31817) could test it on the specific target hardware.
💬 instagibbs commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951081533)
seems to cause quite a few nuisance issues in tests, may be worth it going forward
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951081533)
seems to cause quite a few nuisance issues in tests, may be worth it going forward
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951082137)
That's a more delicate topic. `InferScript` can return `nullptr` in valid scenarios too. It shouldn't just fail in every case as you're doing there.
For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
We need a different way to signal errors from inner `InferScript()` calls. Could be a boolean or.. a different return object like a
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951082137)
That's a more delicate topic. `InferScript` can return `nullptr` in valid scenarios too. It shouldn't just fail in every case as you're doing there.
For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
We need a different way to signal errors from inner `InferScript()` calls. Could be a boolean or.. a different return object like a
...
💬 sipa commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951097542)
Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951097542)
Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
💬 ryanofsky commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2651239390)
> I actually implemented this in [#25297](https://github.com/bitcoin/bitcoin/pull/25297), three years ago :). Will see how much work is needed to revive it next week.
@furszy since I mentioned it the other day, this is a link to the [`PtrOrValue`](https://github.com/ryanofsky/libmultiprocess/blob/ce4814f46d3c38d0bdd08df2253bc69b084f22ee/include/mp/util.h#L136-L148) utility I used in libmultiprocess to allow functions to lock internally or optionally accept an external lock, without requiring tw
...
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2651239390)
> I actually implemented this in [#25297](https://github.com/bitcoin/bitcoin/pull/25297), three years ago :). Will see how much work is needed to revive it next week.
@furszy since I mentioned it the other day, this is a link to the [`PtrOrValue`](https://github.com/ryanofsky/libmultiprocess/blob/ce4814f46d3c38d0bdd08df2253bc69b084f22ee/include/mp/util.h#L136-L148) utility I used in libmultiprocess to allow functions to lock internally or optionally accept an external lock, without requiring tw
...
💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2651275762)
> With those parameters though, BnB should be producing change.
Take a look at coinselection fuzz target. See that regardless the parameters/coins/etc we assert that BnB does not produce change (this is the expected for this algorithm).
```cpp
// Run coinselection algorithms
auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} :
SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight
...
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2651275762)
> With those parameters though, BnB should be producing change.
Take a look at coinselection fuzz target. See that regardless the parameters/coins/etc we assert that BnB does not produce change (this is the expected for this algorithm).
```cpp
// Run coinselection algorithms
auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} :
SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight
...
💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1951154411)
Yes, this would be simpler but I am not sure if we want it to be simpler. With fuzzing we usually try to hit as different many scenarios as possible and that's why I chose this, sometimes setting `block_reserved_weight` and sometimes not, and setting it to a variety of values. I'm not an expert on fuzzing though, maybe someone with a focus on it can weight in here.
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1951154411)
Yes, this would be simpler but I am not sure if we want it to be simpler. With fuzzing we usually try to hit as different many scenarios as possible and that's why I chose this, sometimes setting `block_reserved_weight` and sometimes not, and setting it to a variety of values. I'm not an expert on fuzzing though, maybe someone with a focus on it can weight in here.
💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-2651316273)
The last commit had a small typo, force-pushing to fix it
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-2651316273)
The last commit had a small typo, force-pushing to fix it
🤔 BrandonOdiwuor reviewed a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609296885)
Code Review ACK bb0879ddabc8b3a7253bc774d23b842937d18015
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609296885)
Code Review ACK bb0879ddabc8b3a7253bc774d23b842937d18015
👍 theuni approved a pull request: "guix: remove test-security/symbol-check scripts"
(https://github.com/bitcoin/bitcoin/pull/31818#pullrequestreview-2609339794)
utACK 76c090145e9bb64fe4ef6a663723dd0e9028ed10
(https://github.com/bitcoin/bitcoin/pull/31818#pullrequestreview-2609339794)
utACK 76c090145e9bb64fe4ef6a663723dd0e9028ed10
💬 maflcko commented on pull request "test: Clear scheduler after init to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651420109)
Actually, this is incomplete. It will handle the coverage generated in the notification itself (`f()`), but there is no guarantee that the re-lock is handled as well when the context ends happens before fuzz target execution starts after init. Relevant scope for the re-lock:
https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/scheduler.cpp#L56-L61
The change here should be an improvement, but I wonder if there is a trivial one-line complete solution.
On
...
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651420109)
Actually, this is incomplete. It will handle the coverage generated in the notification itself (`f()`), but there is no guarantee that the re-lock is handled as well when the context ends happens before fuzz target execution starts after init. Relevant scope for the re-lock:
https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/scheduler.cpp#L56-L61
The change here should be an improvement, but I wonder if there is a trivial one-line complete solution.
On
...
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951218506)
> Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
Hmm, I would prefer to avoid returning `addr()` descriptors when the script cannot be redeemed due to consensus rules. Mainly because it opens up the remote possibility of users sending money to it (since the address is there).
This isn't an issue in this `gettxout()` case because the script comes from an unspent outpu
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951218506)
> Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
Hmm, I would prefer to avoid returning `addr()` descriptors when the script cannot be redeemed due to consensus rules. Mainly because it opens up the remote possibility of users sending money to it (since the address is there).
This isn't an issue in this `gettxout()` case because the script comes from an unspent outpu
...
💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651426767)
Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.
Separately (but maybe related?), I ran this script again on `p2p_headers_presync` and there was no failure. If you look at these two coverage reports and go to line 1126, you'll see the different hit counts:
https://marcofleon.github.io/coverage/p2p_headers_presync/coverage/root/bitcoin/src/net_processing.cpp.html
https://marcof
...
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651426767)
Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.
Separately (but maybe related?), I ran this script again on `p2p_headers_presync` and there was no failure. If you look at these two coverage reports and go to line 1126, you'll see the different hit counts:
https://marcofleon.github.io/coverage/p2p_headers_presync/coverage/root/bitcoin/src/net_processing.cpp.html
https://marcof
...
🤔 mzumsande reviewed a pull request: "streams: Add stream position validation in BufferedFile::AdvanceStream"
(https://github.com/bitcoin/bitcoin/pull/31839#pullrequestreview-2609402837)
> The prior assert with the exact same check covers this. This is adding dead code.
`Fill()` may be called in between, which can change `nSrcPos` - but it can only increase it, so yes, still dead code.
(https://github.com/bitcoin/bitcoin/pull/31839#pullrequestreview-2609402837)
> The prior assert with the exact same check covers this. This is adding dead code.
`Fill()` may be called in between, which can change `nSrcPos` - but it can only increase it, so yes, still dead code.
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651462016)
> Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.
I am thinking that sometimes the non-determinism is specific to one input. For example, if a piece of code is completely unreachable by one input, it will never be non-deterministic, so running over that seed to reproduce the non-determinism is just busy-work.
> edit: I guess if the target is unstable then maybe it wouldn't fai
...
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651462016)
> Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.
I am thinking that sometimes the non-determinism is specific to one input. For example, if a piece of code is completely unreachable by one input, it will never be non-deterministic, so running over that seed to reproduce the non-determinism is just busy-work.
> edit: I guess if the target is unstable then maybe it wouldn't fai
...
💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1951234145)
Accidental parentheses at the end here?
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1951234145)
Accidental parentheses at the end here?