💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2384140877)
PR description updated. Created #31000 so we can all run the benchmark on an external HDD in a more friendly manner.
Have seen a 15% speedup locally on a USB 3.2 pendrive.
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2384140877)
PR description updated. Created #31000 so we can all run the benchmark on an external HDD in a more friendly manner.
Have seen a 15% speedup locally on a USB 3.2 pendrive.
🚀 achow101 merged a pull request: "test: switch MiniWallet padding unit from weight to vsize"
(https://github.com/bitcoin/bitcoin/pull/30718)
(https://github.com/bitcoin/bitcoin/pull/30718)
💬 achow101 commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2384156600)
Added release notes (from rev https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft/eab8c9df5996bfe0d0997f747eefbdb745381aa0)
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2384156600)
Added release notes (from rev https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft/eab8c9df5996bfe0d0997f747eefbdb745381aa0)
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781748330)
`i` is not in scope here anymore.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781748330)
`i` is not in scope here anymore.
💬 ffrediani commented on issue "Add IPv6 pinhole support using UPnP / NAT-PMP":
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2384167286)
@laanwj @Sjors I know how UPnP has been controversy over the years (and now PCP maybe inherit this), but what is your view at some point to start having this option enabled by default ? This could enhance significantly the P2P data exchange between peers and reduce a lot the need to use slower and more problematic networks as Tor and I2P, mainly because many residential Broadband connections have working IPv6 and routers that understand it so it will enables a fair amount of nodes to contribute
...
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2384167286)
@laanwj @Sjors I know how UPnP has been controversy over the years (and now PCP maybe inherit this), but what is your view at some point to start having this option enabled by default ? This could enhance significantly the P2P data exchange between peers and reduce a lot the need to use slower and more problematic networks as Tor and I2P, mainly because many residential Broadband connections have working IPv6 and routers that understand it so it will enables a fair amount of nodes to contribute
...
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781761151)
Changed to:
> clusterlin: merge two DepGraph fuzz tests into simulation test
>
> This combines the clusterlin_add_dependency and clusterlin_cluster_serialization fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts from an empty DepGraph and performs a arbitrary number of AddTransaction, AddDependencies, and RemoveTransactions operations on it, and compares the resulting state with a naive reimplementation.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781761151)
Changed to:
> clusterlin: merge two DepGraph fuzz tests into simulation test
>
> This combines the clusterlin_add_dependency and clusterlin_cluster_serialization fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts from an empty DepGraph and performs a arbitrary number of AddTransaction, AddDependencies, and RemoveTransactions operations on it, and compares the resulting state with a naive reimplementation.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781762376)
Changed to:
> /** Read any set of transactions from the provider (including unused positions). */
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781762376)
Changed to:
> /** Read any set of transactions from the provider (including unused positions). */
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781762443)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781762443)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781763047)
Done. I have also added an actual counting of the number of transactions in `real` at the end, and an `assert` to compare it with `num_tx_sim`.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781763047)
Done. I have also added an actual counting of the number of transactions in `real` at the end, and an `assert` to compare it with `num_tx_sim`.
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781734202)
in commit 3c1c9e766ad4fc6defdc9b4814c1e184f6603003: the `num_outputs` parameter is currently unused, so all txs created with this helper only have one output
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781734202)
in commit 3c1c9e766ad4fc6defdc9b4814c1e184f6603003: the `num_outputs` parameter is currently unused, so all txs created with this helper only have one output
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781744846)
was this meant to be
```suggestion
NodeId rand_peer = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, NUM_PEERS - 1);
```
instead, considering that the specified range end is inclusive? the peer loops and asserts have also currently different conditions (`< NUM_PEERS` in `CheckInvariants` vs. `<= NUM_PEERS` in the disconnect loops), so at least one of those needs to be adapted.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781744846)
was this meant to be
```suggestion
NodeId rand_peer = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, NUM_PEERS - 1);
```
instead, considering that the specified range end is inclusive? the peer loops and asserts have also currently different conditions (`< NUM_PEERS` in `CheckInvariants` vs. `<= NUM_PEERS` in the disconnect loops), so at least one of those needs to be adapted.
🤔 jonatack reviewed a pull request: "doc: Minor update to doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2338659503)
ACK 3208df2a100f58569165081cd20c02abed827286
Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2338659503)
ACK 3208df2a100f58569165081cd20c02abed827286
Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
💬 sipa commented on pull request "scripted-diff: Modernize nLocalServices naming":
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384271156)
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384271156)
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
💬 Javadseyedi12 commented on pull request "scripted-diff: Modernize nLocalServices naming":
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384288477)
Hello, good time, yes, what should I do?
در تاریخ دوشنبه ۳۰ سپتامبر ۲۰۲۴، ۱۸:۱۵ Pieter Wuille <
***@***.***> نوشت:
> utACK 33381ea
> <https://github.com/bitcoin/bitcoin/commit/33381ea530ad79ac1e04c37f5707e93d3e0509ca>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384271156>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BLV7G4Q3UB43DPF6OJHTUM3ZZHEQ5AVCNFSM6AAAAABODX6MB6VHI2DSMVQWIX3LMV
...
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384288477)
Hello, good time, yes, what should I do?
در تاریخ دوشنبه ۳۰ سپتامبر ۲۰۲۴، ۱۸:۱۵ Pieter Wuille <
***@***.***> نوشت:
> utACK 33381ea
> <https://github.com/bitcoin/bitcoin/commit/33381ea530ad79ac1e04c37f5707e93d3e0509ca>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2384271156>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BLV7G4Q3UB43DPF6OJHTUM3ZZHEQ5AVCNFSM6AAAAABODX6MB6VHI2DSMVQWIX3LMV
...
💬 sipa commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384291593)
I see both points of view here, and I have a hard time weighing them:
* There are certainly users for whom it is helpful that their Bitcoin P2P connections aren't as easily observable as V1 connection are, by their ISP or other entities, even if no strong hiding can be guaranteed.
* An option like this will likely result in a false sense of security for others, and I'm not comfortable with (eventually, if this were to be made the default) making it harder for older/other software to connect to
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384291593)
I see both points of view here, and I have a hard time weighing them:
* There are certainly users for whom it is helpful that their Bitcoin P2P connections aren't as easily observable as V1 connection are, by their ISP or other entities, even if no strong hiding can be guaranteed.
* An option like this will likely result in a false sense of security for others, and I'm not comfortable with (eventually, if this were to be made the default) making it harder for older/other software to connect to
...
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384295168)
Sorry for the repeated force pushes; I made a change in the wrong commit, and then made some minor improvements to the new fuzz too.
Absent non-nitty review that needs addressing, I don't intend to make change here anymore.
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2384295168)
Sorry for the repeated force pushes; I made a change in the wrong commit, and then made some minor improvements to the new fuzz too.
Absent non-nitty review that needs addressing, I don't intend to make change here anymore.
💬 iotamega commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384325909)
> @jonatack While we should always be vigilant about the possibility of contributors with bad intentions, I think it is more productive to do so by judging contributions by their merit than by trying to infer people's intent.
I think intent here is pretty clear considering I am now working with CloudFlare to stop a DDOS attack on all of my sites.
I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves but I a
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2384325909)
> @jonatack While we should always be vigilant about the possibility of contributors with bad intentions, I think it is more productive to do so by judging contributions by their merit than by trying to infer people's intent.
I think intent here is pretty clear considering I am now working with CloudFlare to stop a DDOS attack on all of my sites.
I asked for this because of a friend in a country where they are having issues, very real issues and they did not want to doxx themselves but I a
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781704314)
nit: same here, I'd remove "all"
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781704314)
nit: same here, I'd remove "all"
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781795191)
I've been thinking about leaking information about private broadcast via addrman:
1.) Here, we call `Good()`, possibly moving the peer from new to tried. Since tried table is smaller and sparsely filled especially for newer nodes, and addrs are picked with 50% probability from new/tried, this usually means that we are more likely to connect to them in the future - maybe via clearnet, maybe via future private connections.
2.) On disconnection, we call `Connected()`, changing the `nTime` of an a
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781795191)
I've been thinking about leaking information about private broadcast via addrman:
1.) Here, we call `Good()`, possibly moving the peer from new to tried. Since tried table is smaller and sparsely filled especially for newer nodes, and addrs are picked with 50% probability from new/tried, this usually means that we are more likely to connect to them in the future - maybe via clearnet, maybe via future private connections.
2.) On disconnection, we call `Connected()`, changing the `nTime` of an a
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781677003)
I would find the ThreadOpenConnection logic (which is already convoluted in master) easier to understand if the private broadcast related logic would be separated more clearly, e.g. this whole thing could be behind some bool `private_broadcast_enabled` so `m_private_broadcast.ShouldOpen` wouldn't be called in normal mode and one wouldn't need to scroll down and verify that this function does nothing then. Of course there is no functional difference, so it may be a matter of taste.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1781677003)
I would find the ThreadOpenConnection logic (which is already convoluted in master) easier to understand if the private broadcast related logic would be separated more clearly, e.g. this whole thing could be behind some bool `private_broadcast_enabled` so `m_private_broadcast.ShouldOpen` wouldn't be called in normal mode and one wouldn't need to scroll down and verify that this function does nothing then. Of course there is no functional difference, so it may be a matter of taste.