💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558678)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204558678)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix.
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204561198)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix. Wasted bandwidth and should result in "Peer %d sent us block transactions for block we weren't expecting\n"
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204561198)
oops, indeed, looks like we'll send off bogus getblocktxns here, if it comes from a preferred peer that's not first. needs a fix. Wasted bandwidth and should result in "Peer %d sent us block transactions for block we weren't expecting\n"
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204563081)
That was already the case. The comment is there to point out that the current request will result in a disconnect (unless a new request is made on time).
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204563081)
That was already the case. The comment is there to point out that the current request will result in a disconnect (unless a new request is made on time).
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204567065)
> Is there a reason for the first commit?
It is because this is the only place where we set the chain timestamps to be in the past. In the form of
```
block_1_timestamp = genesis + 1
block_2_timestamp = median(genesis, block_1) + 1
block_3_timestamp = median(genesis, block_1, block_2) + 1
```
And in the wallet, we use the clock time to set the descriptors/keys creation time.
So, even when we create a wallet prior mining the chain, the wallet will always have a birth time after it. Wh
...
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204567065)
> Is there a reason for the first commit?
It is because this is the only place where we set the chain timestamps to be in the past. In the form of
```
block_1_timestamp = genesis + 1
block_2_timestamp = median(genesis, block_1) + 1
block_3_timestamp = median(genesis, block_1, block_2) + 1
```
And in the wallet, we use the clock time to set the descriptors/keys creation time.
So, even when we create a wallet prior mining the chain, the wallet will always have a birth time after it. Wh
...
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561694264)
Code review ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/355bc6098a7373feb1d59f9651a79e1477d22243
reviewed with `git diff master --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. very straight forward set of changes.
We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561694264)
Code review ACK https://github.com/bitcoin/bitcoin/pull/26969/commits/355bc6098a7373feb1d59f9651a79e1477d22243
reviewed with `git diff master --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. very straight forward set of changes.
We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
💬 mzumsande commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561696153)
> 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 why connections/disconnections aren't logged for inbound peers is that these events are remot
...
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561696153)
> 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 why connections/disconnections aren't logged for inbound peers is that these events are remot
...
💬 brunoerg commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561696565)
> We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
Gonna address it, thanks.
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561696565)
> We have a small bug that you could do in a fixup commit as well perhaps: https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894
Gonna address it, thanks.
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561703430)
Upon further consultation by @fanquake , I've been advised to just PR the change straight. keep this PR as is.
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561703430)
Upon further consultation by @fanquake , I've been advised to just PR the change straight. keep this PR as is.
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204580821)
grandparent still not fixed size :facepalm: edited the assertion to be on the multiple of 10. I think it worked this time.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204580821)
grandparent still not fixed size :facepalm: edited the assertion to be on the multiple of 10. I think it worked this time.
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204581045)
Fixed
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204581045)
Fixed
📝 instagibbs opened a pull request: "Unconditionally return when compact block status == READ_STATUS_FAILED"
(https://github.com/bitcoin/bitcoin/pull/27743)
Fixes https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894 which would have resulted in wasted bandwidth every once in a while.
(https://github.com/bitcoin/bitcoin/pull/27743)
Fixes https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894 which would have resulted in wasted bandwidth every once in a while.
💬 Sjors commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561716007)
Concept ACK. Light preference for splitting this commit into a pure move-only and the (much smaller) change of calling `ProcessCompactBlockTxns` instead of `ProcessMessage`. In the future that makes it easier to follow the history of changes.
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561716007)
Concept ACK. Light preference for splitting this commit into a pure move-only and the (much smaller) change of calling `ProcessCompactBlockTxns` instead of `ProcessMessage`. In the future that makes it easier to follow the history of changes.
💬 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
...