💬 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.
🚀 fanquake merged a pull request: "guix: pass `--enable-initfini-array` to release GCC"
(https://github.com/bitcoin/bitcoin/pull/27153)
(https://github.com/bitcoin/bitcoin/pull/27153)
💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1136724397)
Perhaps `multi_a` for consistency, since `multi` is a valid fragment elsewhere?
Or `Tapscript multisig` if you want to explicitly _not_ refer to the fragment.
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1136724397)
Perhaps `multi_a` for consistency, since `multi` is a valid fragment elsewhere?
Or `Tapscript multisig` if you want to explicitly _not_ refer to the fragment.
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136746362)
> walletpassphrase
If you want to keep coverage for that, you can add it as well (before `walletlock`) with a timeout of `1`, which should override the previous timeout of `999999`?
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136746362)
> walletpassphrase
If you want to keep coverage for that, you can add it as well (before `walletlock`) with a timeout of `1`, which should override the previous timeout of `999999`?
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469699591)
Updated ea278094e9937bf96157161941d38a0c08a0aa4e -> c4f2b6d619f6c0faee5aa3118ade1040105fdd8b ([splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1) -> [splitSystemFs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_1..splitSystemFs_2)) to rename `util/fs.*` to `util/fs_helpers.*`
Added commit dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 ([splitSystemFs_3](https://github.com/The
...
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469699591)
Updated ea278094e9937bf96157161941d38a0c08a0aa4e -> c4f2b6d619f6c0faee5aa3118ade1040105fdd8b ([splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1) -> [splitSystemFs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_1..splitSystemFs_2)) to rename `util/fs.*` to `util/fs_helpers.*`
Added commit dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 ([splitSystemFs_3](https://github.com/The
...
📝 russeree opened a pull request: "rpc: enhanced error message for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
This commit addresses an issue in Bitcoin Core where an incorrect bech32 chain network hrp (prefix) would lead to the address being treated as entirely invalid for both base58 and bech32 decoding, without providing the user with an appropriate error message.
reference
https://github.com/bitcoin/bitcoin/issues/26290
**General Changes**
When a user inputs an incorrect address DecodeDestination now informs the user of the input and expected chain prefixes as well as the network name string.
...
(https://github.com/bitcoin/bitcoin/pull/27260)
This commit addresses an issue in Bitcoin Core where an incorrect bech32 chain network hrp (prefix) would lead to the address being treated as entirely invalid for both base58 and bech32 decoding, without providing the user with an appropriate error message.
reference
https://github.com/bitcoin/bitcoin/issues/26290
**General Changes**
When a user inputs an incorrect address DecodeDestination now informs the user of the input and expected chain prefixes as well as the network name string.
...
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136853031)
@mzumsande,
> I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Yes, that slows down a bit the fast `Select()` (any network is ok). I observed that and think it is ok because it is still super fast with or without the shuffling.
> Why does this need `ALREADY_VISITED_AND_BORING`?
The `for` loop could still repeat the whole array from the start if some address was found but was skipped. `ALREADY_VISITED_
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136853031)
@mzumsande,
> I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Yes, that slows down a bit the fast `Select()` (any network is ok). I observed that and think it is ok because it is still super fast with or without the shuffling.
> Why does this need `ALREADY_VISITED_AND_BORING`?
The `for` loop could still repeat the whole array from the start if some address was found but was skipped. `ALREADY_VISITED_
...
💬 fanquake commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469784160)
Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469784160)
Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136869429)
Yes, but I am not saying to do it (use `std::array` and `.at()`). It is just an option, in case you have not considered it. Up to you.
Btw, a difference between an `assert()` and `.at()` is that the former will call `abort()` and will inevitably cause the program to exit. OTOH `.at()` will throw `std::out_of_range` which the caller can catch and continue the execution. I don't have a strong opinion which one is more preferable to use here. Maybe either one is ok. Usually on such "programmer e
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136869429)
Yes, but I am not saying to do it (use `std::array` and `.at()`). It is just an option, in case you have not considered it. Up to you.
Btw, a difference between an `assert()` and `.at()` is that the former will call `abort()` and will inevitably cause the program to exit. OTOH `.at()` will throw `std::out_of_range` which the caller can catch and continue the execution. I don't have a strong opinion which one is more preferable to use here. Maybe either one is ok. Usually on such "programmer e
...
💬 fanquake commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136867843)
Not sure why this is being removed
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136867843)
Not sure why this is being removed
💬 russeree commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136873703)
This was removed because the linter was complaining about the file permissions and that was the suggestion. I never deliberately changed any perms or ran anything as sudo.
This will be resolved
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136873703)
This was removed because the linter was complaining about the file permissions and that was the suggestion. I never deliberately changed any perms or ran anything as sudo.
This will be resolved
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136880077)
Oh, yes, now it makes sense! I was confused. I should have read the comment like:
"ensure that the new table gets selected even if new_only is false _(because the tried table is empty)_"
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136880077)
Oh, yes, now it makes sense! I was confused. I should have read the comment like:
"ensure that the new table gets selected even if new_only is false _(because the tried table is empty)_"
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136890366)
Yes. Also std methods that take such "indexes" are usually `size_t`, e.g. https://en.cppreference.com/w/cpp/container/array/operator_at
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136890366)
Yes. Also std methods that take such "indexes" are usually `size_t`, e.g. https://en.cppreference.com/w/cpp/container/array/operator_at
💬 russeree commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469815375)
> Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
Will do. No it was not intentional but was a side effect of moving 3 files over scp to a different host. This has been resolved and commits squashed.
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469815375)
> Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
Will do. No it was not intentional but was a side effect of moving 3 files over scp to a different host. This has been resolved and commits squashed.
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469855782)
Updated dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 -> b22cf8d563a469e29c9942b4fd9f93351d8b9766 ([splitSystemFs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_3) -> [splitSystemFs_4](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_3..splitSystemFs_4)) to fix a broken rename and include an additional header in `compat/assumptions.h`. Up until this point `compat/assumptions.h` relied on being i
...
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469855782)
Updated dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 -> b22cf8d563a469e29c9942b4fd9f93351d8b9766 ([splitSystemFs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_3) -> [splitSystemFs_4](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_3..splitSystemFs_4)) to fix a broken rename and include an additional header in `compat/assumptions.h`. Up until this point `compat/assumptions.h` relied on being i
...