💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200632788)
nit: if `vEvictionCandidates` is empty, there's no point (I think, couldn't see any side effects in `EraseLastKElements` or `ProtectEvictionCandidatesByRatio`) executing the next lines so might as well return early? Unless you think this makes the code less clear?
```suggestion
if (vEvictionCandidates.empty()) return std::nullopt;
std::optional<NodeId> force_evict;
if (force) {
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200632788)
nit: if `vEvictionCandidates` is empty, there's no point (I think, couldn't see any side effects in `EraseLastKElements` or `ProtectEvictionCandidatesByRatio`) executing the next lines so might as well return early? Unless you think this makes the code less clear?
```suggestion
if (vEvictionCandidates.empty()) return std::nullopt;
std::optional<NodeId> force_evict;
if (force) {
```
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200865042)
As you [mentioned](https://bitcoincore.reviews/27600#l-165) in the review club, whitelisting just based on ports enables an attacker that discovered which port(s) are whitelisted to take over all (non-whitelisted) inbound connections slots.
Therefore, I think we have to be careful not to accidentally lead people to this footgun. Binding to `127.0.0.1` seems much more prudent to not set a bad example.
```suggestion
self.restart_node(0, extra_args=['-maxconnections=12', '-whitebind=
...
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200865042)
As you [mentioned](https://bitcoincore.reviews/27600#l-165) in the review club, whitelisting just based on ports enables an attacker that discovered which port(s) are whitelisted to take over all (non-whitelisted) inbound connections slots.
Therefore, I think we have to be careful not to accidentally lead people to this footgun. Binding to `127.0.0.1` seems much more prudent to not set a bad example.
```suggestion
self.restart_node(0, extra_args=['-maxconnections=12', '-whitebind=
...
💬 furszy commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1200902347)
little topic:
it seems odd to re-use this two calls in other bench contexts.
`NotifyWalletLoaded` is looping over the wallet load callbacks, and `postInitProcess` syncs the wallet with the mempool, so guess that both will be no-op in most cases?.
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1200902347)
little topic:
it seems odd to re-use this two calls in other bench contexts.
`NotifyWalletLoaded` is looping over the wallet load callbacks, and `postInitProcess` syncs the wallet with the mempool, so guess that both will be no-op in most cases?.
💬 fanquake commented on pull request "depends: remove redundant stdlib option":
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1557725108)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1557725108)
Concept ACK
💬 theStack commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200954342)
Nice indeed, will take it if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200954342)
Nice indeed, will take it if I have to retouch.
💬 theStack commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200970095)
As per #19469 and #20755 it is not possible any more to inspect a peer's ban score via RPC. Could either assert for a more concrete debug log message (containing the expected '(0 -> 20)' banscore before/after string), or even assert for a disconnect after send the problematic message 5 times (100/20), but that all seems excessive and we also don't do that in other functional tests checking misbehavior. As for "assering about not disconnecting", I think that's already covered by using the `send_a
...
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200970095)
As per #19469 and #20755 it is not possible any more to inspect a peer's ban score via RPC. Could either assert for a more concrete debug log message (containing the expected '(0 -> 20)' banscore before/after string), or even assert for a disconnect after send the problematic message 5 times (100/20), but that all seems excessive and we also don't do that in other functional tests checking misbehavior. As for "assering about not disconnecting", I think that's already covered by using the `send_a
...
💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1201120763)
Yes, I think they'll be no-op for the most part.
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1201120763)
Yes, I think they'll be no-op for the most part.
👍 theStack approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1437730435)
re-ACK 4eee95e57bb6f773bcaeb405bca949f158a62134
Left two comments below, one nit and one "thinking out loud what could happen in the worst case", both no stoppers.
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1437730435)
re-ACK 4eee95e57bb6f773bcaeb405bca949f158a62134
Left two comments below, one nit and one "thinking out loud what could happen in the worst case", both no stoppers.
💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201199915)
nit, if you need to retouch:
```suggestion
const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
```
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201199915)
nit, if you need to retouch:
```suggestion
const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
```
💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201230428)
No stopper, but I think there should be really a good reason to remove existing unit-test checks. For example, if (for whatever reason) `LookupHost` fails and hence returns std::nullopt, we'd now get a more cryptic error message without seeing the direct line number of the cause:
```
unknown location(0): fatal error: in "net_tests/cnetaddr_basic": std::bad_optional_access: bad_optional_access
test/net_tests.cpp(133): last checkpoint: "cnetaddr_basic" test entry
```
compared to an explicit
...
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201230428)
No stopper, but I think there should be really a good reason to remove existing unit-test checks. For example, if (for whatever reason) `LookupHost` fails and hence returns std::nullopt, we'd now get a more cryptic error message without seeing the direct line number of the cause:
```
unknown location(0): fatal error: in "net_tests/cnetaddr_basic": std::bad_optional_access: bad_optional_access
test/net_tests.cpp(133): last checkpoint: "cnetaddr_basic" test entry
```
compared to an explicit
...
⚠️ Crypt-iQ opened an issue: "bitcoind hangs waiting for `g_requests.empty()`"
(https://github.com/bitcoin/bitcoin/issues/27722)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
bitcoind hangs
### Expected behaviour
bitcoind shouldn't hang
I was able to dump the running threads and it was stuck waiting for `g_requests` to be empty. The issue seems to be that this callback may not actually be called in case of error: https://github.com/bitcoin/bitcoin/blob/22139f6e83a2bedd2dad9f280567d2c76c54252f/src/httpserver.cpp#L222-L227
See this issue https://github.c
...
(https://github.com/bitcoin/bitcoin/issues/27722)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
bitcoind hangs
### Expected behaviour
bitcoind shouldn't hang
I was able to dump the running threads and it was stuck waiting for `g_requests` to be empty. The issue seems to be that this callback may not actually be called in case of error: https://github.com/bitcoin/bitcoin/blob/22139f6e83a2bedd2dad9f280567d2c76c54252f/src/httpserver.cpp#L222-L227
See this issue https://github.c
...
💬 theStack commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1558205497)
>>So far so good, everything went quite smooth and nothing unexpected happened! The only disappointment was finding that apparently the provided prune target of 800MB was not respected:
>
>Yes, this is unfortunately expected behavior. Since you have -blockfilterindex enabled and the background sync hasn't yet completed, prune locks are preventing any pruning from happening on the snapshot chainstate. I mentioned this obliquely here: https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-15
...
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1558205497)
>>So far so good, everything went quite smooth and nothing unexpected happened! The only disappointment was finding that apparently the provided prune target of 800MB was not respected:
>
>Yes, this is unfortunately expected behavior. Since you have -blockfilterindex enabled and the background sync hasn't yet completed, prune locks are preventing any pruning from happening on the snapshot chainstate. I mentioned this obliquely here: https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-15
...
💬 BebeSparkelSparkel commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1558482031)
I will try again when I get the chance and let you know.
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1558482031)
I will try again when I get the chance and let you know.
💬 chippsmith commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1558495158)
I downloaded the binaries on a System76 machine running Pop!OS 22.04 LTS. I ran all the tests in the testing guide(except the last one about verifying binaries) and got expected results. More detailed notes can be found here. https://github.com/chippsmith/bitcoinnotes/tree/main/bitcoin25.0rc.
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1558495158)
I downloaded the binaries on a System76 machine running Pop!OS 22.04 LTS. I ran all the tests in the testing guide(except the last one about verifying binaries) and got expected results. More detailed notes can be found here. https://github.com/chippsmith/bitcoinnotes/tree/main/bitcoin25.0rc.
⚠️ CryptoManiac opened an issue: "Validation of malformed address fails with peculiar message"
(https://github.com/bitcoin/bitcoin/issues/27723)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Open RPC console and execute this command:
`validateaddress bc1qqrq69gfzvvqcxs6rgg3crqjzcw369sxzyp3v9sspursx9gmzyv32x7xa5z`
Following message is the result:
```
Internal bug detected: 'isValid == error_msg.empty()'
rpc/misc.cpp:75 (operator())
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
(code -1)
```
### Expected behaviour
Well it should tel
...
(https://github.com/bitcoin/bitcoin/issues/27723)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Open RPC console and execute this command:
`validateaddress bc1qqrq69gfzvvqcxs6rgg3crqjzcw369sxzyp3v9sspursx9gmzyv32x7xa5z`
Following message is the result:
```
Internal bug detected: 'isValid == error_msg.empty()'
rpc/misc.cpp:75 (operator())
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
(code -1)
```
### Expected behaviour
Well it should tel
...
💬 de-served commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1558537251)
> My recommendation for you personally would be to configure the GUI to use the default datadir and place your `bitcoin.conf` file in `C:\Users\USERNAME\AppData\Roaming\Bitcoin` instead of
It's already here... for a "debug" what's happening:
`
datadir =W:/BitcoinCore/DataDir_/_User
blocksdir =W:/BitcoinCore/BlocksDir_/_User
walletdir =W:/BitcoinCore/WalletDir_/_User
debuglogfile=H:/_Logs_/BitcoinCore/_debug.default-appdata.no_params.log
[main]
datadir =W:/BitcoinCore/Data
...
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1558537251)
> My recommendation for you personally would be to configure the GUI to use the default datadir and place your `bitcoin.conf` file in `C:\Users\USERNAME\AppData\Roaming\Bitcoin` instead of
It's already here... for a "debug" what's happening:
`
datadir =W:/BitcoinCore/DataDir_/_User
blocksdir =W:/BitcoinCore/BlocksDir_/_User
walletdir =W:/BitcoinCore/WalletDir_/_User
debuglogfile=H:/_Logs_/BitcoinCore/_debug.default-appdata.no_params.log
[main]
datadir =W:/BitcoinCore/Data
...
💬 MarcoFalke commented on pull request "test: Wallet interactions with rescanning wallet":
(https://github.com/bitcoin/bitcoin/pull/26700#discussion_r1201575251)
This will race against the thread `t`, which may have already concluded before this point
(https://github.com/bitcoin/bitcoin/pull/26700#discussion_r1201575251)
This will race against the thread `t`, which may have already concluded before this point
💬 MarcoFalke commented on issue "Validation of malformed address fails with a peculiar message":
(https://github.com/bitcoin/bitcoin/issues/27723#issuecomment-1558620137)
Thank you for the bug report. I think what happens is:
* The function `ConvertBits<5, 8, false>` fails, however
* the function `bech32::LocateErrors` also fails to locate any errors.
Thus the `CHECK_NONFATAL(isValid == error_msg.empty())` added in commit 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e will be hit.
(https://github.com/bitcoin/bitcoin/issues/27723#issuecomment-1558620137)
Thank you for the bug report. I think what happens is:
* The function `ConvertBits<5, 8, false>` fails, however
* the function `bech32::LocateErrors` also fails to locate any errors.
Thus the `CHECK_NONFATAL(isValid == error_msg.empty())` added in commit 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e will be hit.
💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1201618467)
You can just use `DataStream` for that, which comes with compile-time checks to enforce that both values are in fact never read, and thus can be omitted completely.
Also, in the same commit, is there a reason why you are changing `DataStream ssKey` to `CDataStream ssKey`? That seems like a step backward, no?
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1201618467)
You can just use `DataStream` for that, which comes with compile-time checks to enforce that both values are in fact never read, and thus can be omitted completely.
Also, in the same commit, is there a reason why you are changing `DataStream ssKey` to `CDataStream ssKey`? That seems like a step backward, no?
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1558676764)
> Has that idea been discussed somewhere already?
The idea was published quite a while ago, but I learned about it just recently.
We looked around to see if there were existing discussions or implementations, but we didn't find anything on bitcoin-dev or the repo here.
There's an old implementation in a Bitcoin fork (Drivechain) which I used as a starting point, but ended up with a pretty different approach and implementation.
We were thinking of perhaps writing down the spec into a Pro
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1558676764)
> Has that idea been discussed somewhere already?
The idea was published quite a while ago, but I learned about it just recently.
We looked around to see if there were existing discussions or implementations, but we didn't find anything on bitcoin-dev or the repo here.
There's an old implementation in a Bitcoin fork (Drivechain) which I used as a starting point, but ended up with a pretty different approach and implementation.
We were thinking of perhaps writing down the spec into a Pro
...