💬 achow101 commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561716951)
> Do you mean moving some of the core logic inside the interfaces::Wallet API, or the CWallet implementation?
Inside of `CWallet`.
> Since this was my first Bitcoin Core change I decided to be more conservative and avoid API changes.
> Of course, if this PR is accepted, I'd be happy to move functionality into the API.
The API isn't public, so changing it isn't a big concern. Really the only consumer of it is the GUI.
Having it inside of `CWallet` will probably make this feature more
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561716951)
> Do you mean moving some of the core logic inside the interfaces::Wallet API, or the CWallet implementation?
Inside of `CWallet`.
> Since this was my first Bitcoin Core change I decided to be more conservative and avoid API changes.
> Of course, if this PR is accepted, I'd be happy to move functionality into the API.
The API isn't public, so changing it isn't a big concern. Really the only consumer of it is the GUI.
Having it inside of `CWallet` will probably make this feature more
...
💬 theStack commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1561718193)
> I was able to fix this with my branch here [Crypt-iQ@4fd7adb](https://github.com/Crypt-iQ/bitcoin/commit/4fd7adb30f584664421b6431bce8ebcbf7ceef2f) by adding a `evhttp_connection_set_closecb` callback. I didn't need to modify the logic related to `EV_READ` and no functional test errors are introduced on my machine
Can confirm that this patch fixes the issue on my side.
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1561718193)
> I was able to fix this with my branch here [Crypt-iQ@4fd7adb](https://github.com/Crypt-iQ/bitcoin/commit/4fd7adb30f584664421b6431bce8ebcbf7ceef2f) by adding a `evhttp_connection_set_closecb` callback. I didn't need to modify the logic related to `EV_READ` and no functional test errors are introduced on my machine
Can confirm that this patch fixes the issue on my side.
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204590378)
#27743
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204590378)
#27743
💬 Sjors commented on pull request "Unconditionally return when compact block status == READ_STATUS_FAILED":
(https://github.com/bitcoin/bitcoin/pull/27743#issuecomment-1561719535)
utACK d97269579769effbe6eec2303ea0cc3e396d3e0d
(https://github.com/bitcoin/bitcoin/pull/27743#issuecomment-1561719535)
utACK d97269579769effbe6eec2303ea0cc3e396d3e0d
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561725613)
> The API isn't public, so changing it isn't a big concern. Really the only consumer of it is the GUI.
>
> Having it inside of `CWallet` will probably make this feature more likely to be accepted. It will be easier to write tests (adding an RPC allows for functional tests, and implementing in `CWallet` allows unit tests to better access the data) and it can be split up into at least 2 chunks - the `CWallet` and RPC only implementation, and then the GUI and interface changes. This makes it mor
...
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561725613)
> The API isn't public, so changing it isn't a big concern. Really the only consumer of it is the GUI.
>
> Having it inside of `CWallet` will probably make this feature more likely to be accepted. It will be easier to write tests (adding an RPC allows for functional tests, and implementing in `CWallet` allows unit tests to better access the data) and it can be split up into at least 2 chunks - the `CWallet` and RPC only implementation, and then the GUI and interface changes. This makes it mor
...
💬 D33r-Gee commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1561739681)
Tested up to "Finalizing a PSBT..." (no bonus test yet).
They all executed successfully!
25.0rc2 compiled from source
Hardware Info:
- Memory: 15.5 GiB
- Processor: Intel® Core™ i7-7700HQ CPU @ 2.80GHz × 8
- OS: Ubuntu 20.04.5 LTS 64-bit
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1561739681)
Tested up to "Finalizing a PSBT..." (no bonus test yet).
They all executed successfully!
25.0rc2 compiled from source
Hardware Info:
- Memory: 15.5 GiB
- Processor: Intel® Core™ i7-7700HQ CPU @ 2.80GHz × 8
- OS: Ubuntu 20.04.5 LTS 64-bit
💬 achow101 commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561747032)
> I'd need to open a PR against the main repo in that case, right?
Yes
> Should I keep this PR and leave the GUI bits here, and open another PR in the main repo with just the CWallet changes?
> Or is it better to close this PR and open a new one in the main repo with all the changes?
You can leave this open and rebase it onto the main repo PR, just mention it in the description.
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561747032)
> I'd need to open a PR against the main repo in that case, right?
Yes
> Should I keep this PR and leave the GUI bits here, and open another PR in the main repo with just the CWallet changes?
> Or is it better to close this PR and open a new one in the main repo with all the changes?
You can leave this open and rebase it onto the main repo PR, just mention it in the description.
💬 ArmchairCryptologist commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561755480)
>The reason why connections/disconnections aren't logged for inbound peers is that these events are remotely triggerable at almost no cost. If we'd log these by default, that would make us susceptible to disk-exhaustion attacks where an attacker tries to fill up our logs over time by triggering the log repeatedly.
I see, that makes sense in general. But when a connection is closed due to a block download timeout, you already generate a log entry `Timeout downloading block x from peer=y, disco
...
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561755480)
>The reason why connections/disconnections aren't logged for inbound peers is that these events are remotely triggerable at almost no cost. If we'd log these by default, that would make us susceptible to disk-exhaustion attacks where an attacker tries to fill up our logs over time by triggering the log repeatedly.
I see, that makes sense in general. But when a connection is closed due to a block download timeout, you already generate a log entry `Timeout downloading block x from peer=y, disco
...
💬 davidsfeet7-z commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561760539)
Please stop
On Wed, May 24, 2023, 12:56 PM Martin Zumsande ***@***.***>
wrote:
> I did block the peer right after I posted the latest logs four days ago,
> and after that the block timeouts dropped to 2-3 per day, but like I
> mentioned in the bug report, since Core doesn't normally log the IP of
> disconnected incoming peers you pretty much have to enable debug=net
> logging to find the IP of the misbehaving peer. Which on this particular
> node is ~1GB of logs per 2-3 hours.
>
> The reason w
...
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561760539)
Please stop
On Wed, May 24, 2023, 12:56 PM Martin Zumsande ***@***.***>
wrote:
> I did block the peer right after I posted the latest logs four days ago,
> and after that the block timeouts dropped to 2-3 per day, but like I
> mentioned in the bug report, since Core doesn't normally log the IP of
> disconnected incoming peers you pretty much have to enable debug=net
> logging to find the IP of the misbehaving peer. Which on this particular
> node is ~1GB of logs per 2-3 hours.
>
> The reason w
...
🤔 mzumsande reviewed a pull request: "Unconditionally return when compact block status == READ_STATUS_FAILED"
(https://github.com/bitcoin/bitcoin/pull/27743#pullrequestreview-1442557635)
ACK d97269579769effbe6eec2303ea0cc3e396d3e0d
Agree that it doesn't make sense to continue and send a BLOCKTXN here.
(https://github.com/bitcoin/bitcoin/pull/27743#pullrequestreview-1442557635)
ACK d97269579769effbe6eec2303ea0cc3e396d3e0d
Agree that it doesn't make sense to continue and send a BLOCKTXN here.
💬 willcl-ark commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-1561803409)
> I have opened a draft PR #27731 that uses the proposed solution with the max connection feature that was added there. But currently I still have a hard time reproducing the issue in the same way I was able to back in 2020 described above: [#11368 (comment)](https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-638770701) I have a much faster machine now but there may also be other reasons.
I also do not seem to be able to reproduce this on master with a similar python script.
<det
...
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-1561803409)
> I have opened a draft PR #27731 that uses the proposed solution with the max connection feature that was added there. But currently I still have a hard time reproducing the issue in the same way I was able to back in 2020 described above: [#11368 (comment)](https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-638770701) I have a much faster machine now but there may also be other reasons.
I also do not seem to be able to reproduce this on master with a similar python script.
<det
...
⚠️ Sjors opened an issue: "Log which peer sent us a header (first)"
(https://github.com/bitcoin/bitcoin/issues/27744)
Discussed in https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1478368582
```
Saw new header hash=... height=...
```
It would be useful to add the node id to this message. The validation code doesn't know which peer the header came from, so we'd have to move the log entry to net_processing.
One way is to add a helper function `LogBlockHeader` and call it right after both `ProcessNewBlockHeaders(header, peer, compact_blk)` calls in net_processing, only if they return true. Sin
...
(https://github.com/bitcoin/bitcoin/issues/27744)
Discussed in https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1478368582
```
Saw new header hash=... height=...
```
It would be useful to add the node id to this message. The validation code doesn't know which peer the header came from, so we'd have to move the log entry to net_processing.
One way is to add a helper function `LogBlockHeader` and call it right after both `ProcessNewBlockHeaders(header, peer, compact_blk)` calls in net_processing, only if they return true. Sin
...
📝 amitiuttarwar opened a pull request: "addrman: select addresses by network follow-up "
(https://github.com/bitcoin/bitcoin/pull/27745)
this PR addresses outstanding review comments from #27214
(https://github.com/bitcoin/bitcoin/pull/27745)
this PR addresses outstanding review comments from #27214
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661115)
done in #27745
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661115)
done in #27745
💬 mzumsande commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561807894)
> As such it shouldn't add any risk of such attacks if the remote address was simply added to this line, something like peer=x addr=x.x.x.x?
I agree, with `addr=x.x.x.x` being conditional on `-logips`. That could make sense in some more spots as well, I could open a PR to suggest it unless you want to.
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561807894)
> As such it shouldn't add any risk of such attacks if the remote address was simply added to this line, something like peer=x addr=x.x.x.x?
I agree, with `addr=x.x.x.x` being conditional on `-logips`. That could make sense in some more spots as well, I could open a PR to suggest it unless you want to.
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661204)
done in #27745
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661204)
done in #27745
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661284)
done in #27745
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661284)
done in #27745
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661358)
done in #27745
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661358)
done in #27745
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661423)
done in #27745
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1204661423)
done in #27745
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1204663303)
Good catch thanks. First draft of this PR did check for `blocks` directory but of course that is unreliable because it could be customized by the user.
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1204663303)
Good catch thanks. First draft of this PR did check for `blocks` directory but of course that is unreliable because it could be customized by the user.