💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608255803)
Yes, `iters->by_txid->first` is the same as `txid`, changed it to use `txid` which is shorter.
It is necessary to remove and re-add because we have changed the key and C++ maps do not support modifying of the keys. That is, changing the key means that the entry will have to be in another place in the map, so it has to be removed from the old place and added to the new one.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608255803)
Yes, `iters->by_txid->first` is the same as `txid`, changed it to use `txid` which is shorter.
It is necessary to remove and re-add because we have changed the key and C++ maps do not support modifying of the keys. That is, changing the key means that the entry will have to be in another place in the map, so it has to be removed from the old place and added to the new one.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2122543090)
> @laanwj This is a router provided by Verizon (a large US ISP) in 2021.
That's curious, especially as NAT-PMP doesn't have any IPv6 support. i'm assuming you don't have any output for IPv6?
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2122543090)
> @laanwj This is a router provided by Verizon (a large US ISP) in 2021.
That's curious, especially as NAT-PMP doesn't have any IPv6 support. i'm assuming you don't have any output for IPv6?
💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608263692)
I don't think we'd ever need to make `TxDownloadManager` depend on `ChainstateManager` as we could just pass in the block hash, i.e.
> Another alternative could be to directly pass the current block hash as a param into the relevant TxDownloadManager methods.
This would avoid the addition of `UpdateBlockTipSync` etc, as it's basically what we do now. I find this way of keeping synchronized with the chain tip pretty ugly - we'd need to hold `cs_main` and pass the blockhash to the `TxDownloa
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608263692)
I don't think we'd ever need to make `TxDownloadManager` depend on `ChainstateManager` as we could just pass in the block hash, i.e.
> Another alternative could be to directly pass the current block hash as a param into the relevant TxDownloadManager methods.
This would avoid the addition of `UpdateBlockTipSync` etc, as it's basically what we do now. I find this way of keeping synchronized with the chain tip pretty ugly - we'd need to hold `cs_main` and pass the blockhash to the `TxDownloa
...
👍 hebasto approved a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2068533600)
ACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af.
Guix build:
```
x86_64
b5dd1ec801cb549590bb922867b9254de51c3688220ce4d29ae03077e0047448 guix-build-17fe948cce2e/output/dist-archive/bitcoin-17fe948cce2e.tar.gz
9bc0b56bf0440f04937646ded534e47df860c4ca2b9b96699c1f5b431ae71508 guix-build-17fe948cce2e/output/x86_64-w64-mingw32/SHA256SUMS.part
3e1cceb01e86b7e47d68ae100b60747333a88aad5b352c7c3e9113d700465f1c guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-debug.zip
...
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2068533600)
ACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af.
Guix build:
```
x86_64
b5dd1ec801cb549590bb922867b9254de51c3688220ce4d29ae03077e0047448 guix-build-17fe948cce2e/output/dist-archive/bitcoin-17fe948cce2e.tar.gz
9bc0b56bf0440f04937646ded534e47df860c4ca2b9b96699c1f5b431ae71508 guix-build-17fe948cce2e/output/x86_64-w64-mingw32/SHA256SUMS.part
3e1cceb01e86b7e47d68ae100b60747333a88aad5b352c7c3e9113d700465f1c guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-debug.zip
...
💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608266669)
Ok, so assuming this was fixed, it can probably be reverted some time next year, given the EOL policy https://www.freebsd.org/security/#sup
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608266669)
Ok, so assuming this was fixed, it can probably be reverted some time next year, given the EOL policy https://www.freebsd.org/security/#sup
💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608272072)
Hm, in the previous setup, we can change any of txdownload_impl.cpp without net_processing.cpp noticing, but yeah maybe less clean to depend on txdownload_impl.h.
I've updated with a new setup where I've added a txdownloadman.cpp depending on txdownload_impl.h, and txdownloadman.h is included by txdownload_impl.h instead of the other way around. And so net_processing.cpp no longer depends on txdownload_impl.h, which makes sense to me. Had to update `lint-circular-dependencies.py` but I do lik
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608272072)
Hm, in the previous setup, we can change any of txdownload_impl.cpp without net_processing.cpp noticing, but yeah maybe less clean to depend on txdownload_impl.h.
I've updated with a new setup where I've added a txdownloadman.cpp depending on txdownload_impl.h, and txdownloadman.h is included by txdownload_impl.h instead of the other way around. And so net_processing.cpp no longer depends on txdownload_impl.h, which makes sense to me. Had to update `lint-circular-dependencies.py` but I do lik
...
🤔 fjahr reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2068538362)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2068538362)
Concept ACK
💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608272490)
nit, I think this wording would be more clear: "The ASN mapped to the IP of this peer per our current ASMap." (and something corresponding to this below).
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608272490)
nit, I think this wording would be more clear: "The ASN mapped to the IP of this peer per our current ASMap." (and something corresponding to this below).
💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608267929)
I would prefer these to be optional results rather than a magic 0 value.
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608267929)
I would prefer these to be optional results rather than a magic 0 value.
💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608281308)
> I would prefer these to be optional results rather than a magic 0 value.
(if there is no mapping of course)
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608281308)
> I would prefer these to be optional results rather than a magic 0 value.
(if there is no mapping of course)
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608284734)
Yes, if there is only one transaction submitted for broadcast. But if there is more than one, then `m_private_broadcast_connections_to_open` should not be set to `0`.
For example:
* 3 transactions are submitted at approximately the same time: tx1, tx2, tx3.
* NUM_PRIVATE_BROADCAST_PER_TX=5 for each one, so `m_private_broadcast_connections_to_open` is set to 15.
* tx1 is broadcast 2 times, tx2 once and tx3 once, `m_private_broadcast_connections_to_open` is now 15-2-1-1=11.
* tx1 is receive
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608284734)
Yes, if there is only one transaction submitted for broadcast. But if there is more than one, then `m_private_broadcast_connections_to_open` should not be set to `0`.
For example:
* 3 transactions are submitted at approximately the same time: tx1, tx2, tx3.
* NUM_PRIVATE_BROADCAST_PER_TX=5 for each one, so `m_private_broadcast_connections_to_open` is set to 15.
* tx1 is broadcast 2 times, tx2 once and tx3 once, `m_private_broadcast_connections_to_open` is now 15-2-1-1=11.
* tx1 is receive
...
💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608289449)
I think not using `thread_local` vars with destructors is a reasonable policy anyway, regardless of where it's supported.
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608289449)
I think not using `thread_local` vars with destructors is a reasonable policy anyway, regardless of where it's supported.
💬 maflcko commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122586712)
utACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122586712)
utACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608293544)
Right! Removed.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608293544)
Right! Removed.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608306032)
Changed to `"priv"`.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608306032)
Changed to `"priv"`.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608308448)
Done
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608308448)
Done
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608310055)
I removed the argument to this function and renamed it to `ProxyForIPv4or6PrivateBroadcast()`. It is a bit more simpler now.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608310055)
I removed the argument to this function and renamed it to `ProxyForIPv4or6PrivateBroadcast()`. It is a bit more simpler now.
💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608310558)
I see, so the comment should say: "While this particular bug has been fixed, `thread_local` and especially `thread_local` with non-POD objects should be avoided."?
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608310558)
I see, so the comment should say: "While this particular bug has been fixed, `thread_local` and especially `thread_local` with non-POD objects should be avoided."?
👍 TheCharlatan approved a pull request: "rpc: Optimize serialization and enhance metadata of dumptxoutset output"
(https://github.com/bitcoin/bitcoin/pull/29612#pullrequestreview-2068620308)
Re-ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
(https://github.com/bitcoin/bitcoin/pull/29612#pullrequestreview-2068620308)
Re-ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2122621311)
`65ba8d0203...e85cc59bad`: rebase, address suggestions and remove the argument of `ProxyForClearnetPrivateBroadcast()` because it is always IPv4 or IPv6 and we treat both in the same way.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2122621311)
`65ba8d0203...e85cc59bad`: rebase, address suggestions and remove the argument of `ProxyForClearnetPrivateBroadcast()` because it is always IPv4 or IPv6 and we treat both in the same way.