💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1223106639)
Can you elaborate on this concern? The orphan that isn't received yet may still be in orphanage, and will be connected to its parents normally?
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1223106639)
Can you elaborate on this concern? The orphan that isn't received yet may still be in orphanage, and will be connected to its parents normally?
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1582729266)
`2eae1f8f13...ff88e08363`: address suggestions
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1582729266)
`2eae1f8f13...ff88e08363`: address suggestions
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223152947)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223152947)
Fixed.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223163824)
Right, I relaxed the condition a bit to error only if we are sure Tor is unreachable and will remain unreachable. There is still a chance that the tor control thread will not set Tor to reachable, but there is also a chance that Tor is set to reachable here, but the Tor server is down. Later at runtime we do check again the reachable networks and in general the code must work when some or all of those networks go down.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223163824)
Right, I relaxed the condition a bit to error only if we are sure Tor is unreachable and will remain unreachable. There is still a chance that the tor control thread will not set Tor to reachable, but there is also a chance that Tor is set to reachable here, but the Tor server is down. Later at runtime we do check again the reachable networks and in general the code must work when some or all of those networks go down.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223166293)
Right, I relaxed the condition a bit to error only if we are sure Tor is unreachable and will remain unreachable. There is still a chance that the tor control thread will not set Tor to reachable, but there is also a chance that Tor is set to reachable here, but the Tor server is down. Later at runtime we do check again the reachable networks and in general the code must work when some or all of those networks go down.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223166293)
Right, I relaxed the condition a bit to error only if we are sure Tor is unreachable and will remain unreachable. There is still a chance that the tor control thread will not set Tor to reachable, but there is also a chance that Tor is set to reachable here, but the Tor server is down. Later at runtime we do check again the reachable networks and in general the code must work when some or all of those networks go down.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223171823)
Yes! I changed this to not try 2 consecutive sensitive relay connections. That is - after attempting one, give way to others (if any, otherwise sleep 0.5s) and only then try another sensitive relay.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223171823)
Yes! I changed this to not try 2 consecutive sensitive relay connections. That is - after attempting one, give way to others (if any, otherwise sleep 0.5s) and only then try another sensitive relay.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223178234)
Yes, at least I should raise the fd limit, but by how much? Should we limit the number of sensitive relay connections? I think it makes sense to try as hard as possible even if that means sometimes a temporary error of running out of file descriptors. Or is that not a good idea? I guess in some cases a node operator may submit many transactions at once to their node. What do you think would be the best approach?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1223178234)
Yes, at least I should raise the fd limit, but by how much? Should we limit the number of sensitive relay connections? I think it makes sense to try as hard as possible even if that means sometimes a temporary error of running out of file descriptors. Or is that not a good idea? I guess in some cases a node operator may submit many transactions at once to their node. What do you think would be the best approach?
💬 virtu commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1582791757)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1582791757)
Concept ACK.
🤔 furszy reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1470217367)
Thanks @ryanofsky! I see most of the changes great, except one.
If we change `AppInit` return value to be an int, we need to change all the
return lines to return an int, not only the ones at the end of the function.
Otherwise, the `return false` statements will be treated as `return 0` which
means `EXIT_SUCCESS`, which is the opposite that we want.
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1470217367)
Thanks @ryanofsky! I see most of the changes great, except one.
If we change `AppInit` return value to be an int, we need to change all the
return lines to return an int, not only the ones at the end of the function.
Otherwise, the `return false` statements will be treated as `return 0` which
means `EXIT_SUCCESS`, which is the opposite that we want.
💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1582902399)
Oh, you are right. I thought I searched for early returns, but somehow I missed seeing them. I updated the suggestion above to fix this. I think AppInit should keep returning bool not int to keep the changes minimal, and because returning bool is better for following existing coding conventions.
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1582902399)
Oh, you are right. I thought I searched for early returns, but somehow I missed seeing them. I updated the suggestion above to fix this. I think AppInit should keep returning bool not int to keep the changes minimal, and because returning bool is better for following existing coding conventions.
💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1223330364)
nit:
```suggestion
const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV4)};
```
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1223330364)
nit:
```suggestion
const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV4)};
```
💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1583051690)
Concept ACK, but I'm not conviced about the approach.
In 9b1d32e203435a62caef7079fe8c49f755b43e3d "[Net: Add continuous ASMap health check logging](https://github.com/bitcoin/bitcoin/pull/27581/commits/9b1d32e203435a62caef7079fe8c49f755b43e3d)": You're basically fetching all addresses (ipv4/ipv6) from addrman and doing a sanity check every 24hs. Considering we don't change asmap file frequently (we need to restart the node to do it), do we really need to do it for all addresses every 24hs?
...
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1583051690)
Concept ACK, but I'm not conviced about the approach.
In 9b1d32e203435a62caef7079fe8c49f755b43e3d "[Net: Add continuous ASMap health check logging](https://github.com/bitcoin/bitcoin/pull/27581/commits/9b1d32e203435a62caef7079fe8c49f755b43e3d)": You're basically fetching all addresses (ipv4/ipv6) from addrman and doing a sanity check every 24hs. Considering we don't change asmap file frequently (we need to restart the node to do it), do we really need to do it for all addresses every 24hs?
...
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1223373778)
okay, going to resolve this thread. but lmk if you have any other suggestions, it's definitely a bit odd..
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1223373778)
okay, going to resolve this thread. but lmk if you have any other suggestions, it's definitely a bit odd..
📝 aguycalled opened a pull request: "BLSCT SubAddress Pool"
(https://github.com/bitcoin/bitcoin/pull/27839)
This PR adds support for the use of a SubAddress pool and the generation of different receiving addresses.
`getnewaddress` RPC method pulls addresses from the pool and shows to the requester receiving addresses.
By default it generates addresses under account `0`.
It also introduces changes in `blast::PublicKey` to store the underlying point as a `MclG1Point` object, instead of a byte vector, since deserialisation of the vector is a expensive operation.
(https://github.com/bitcoin/bitcoin/pull/27839)
This PR adds support for the use of a SubAddress pool and the generation of different receiving addresses.
`getnewaddress` RPC method pulls addresses from the pool and shows to the requester receiving addresses.
By default it generates addresses under account `0`.
It also introduces changes in `blast::PublicKey` to store the underlying point as a `MclG1Point` object, instead of a byte vector, since deserialisation of the vector is a expensive operation.
✅ aguycalled closed a pull request: "BLSCT SubAddress Pool"
(https://github.com/bitcoin/bitcoin/pull/27839)
(https://github.com/bitcoin/bitcoin/pull/27839)
💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1583146747)
> My idea currently is to run the analysis once per day
Instead of creating `m_last_asmap_health_check` and putting the logic into `ThreadOpenConnection`. Why not creating `ASMapHealthCheck` in `CConnman`:
```cpp
void CConnman::ASMapHealthCheck()
{
const std::vector<CAddress> v4_addrs{GetAddresses(0, 0, Network::NET_IPV4)};
const std::vector<CAddress> v6_addrs{GetAddresses(0, 0, Network::NET_IPV6)};
std::vector<CNetAddr> clearnet_addrs;
clearnet_addrs.reserve(v4_addrs
...
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1583146747)
> My idea currently is to run the analysis once per day
Instead of creating `m_last_asmap_health_check` and putting the logic into `ThreadOpenConnection`. Why not creating `ASMapHealthCheck` in `CConnman`:
```cpp
void CConnman::ASMapHealthCheck()
{
const std::vector<CAddress> v4_addrs{GetAddresses(0, 0, Network::NET_IPV4)};
const std::vector<CAddress> v6_addrs{GetAddresses(0, 0, Network::NET_IPV6)};
std::vector<CNetAddr> clearnet_addrs;
clearnet_addrs.reserve(v4_addrs
...
💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1223468703)
To avoid calling `GetMappedAS` again
```suggestion
clearnet_asns.insert(asn);
```
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1223468703)
To avoid calling `GetMappedAS` again
```suggestion
clearnet_asns.insert(asn);
```
💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1583255041)
Thanks ryanofsky, updated per feedback with a tiny diff:
Didn't move `InitShutdownState` from `AppInitBasicSetup` to `AppInit` and instead provided the `NodeContext` ref to `AppInitBasicSetup`.
Not sure if you have an strong opinion about this (shoot if you have it) but the rationale was to keep the same workflow as we have now so the bitcoin-qt related changes b0267f2 doesn't introduce another `InitShutdownState` call.
> I think AppInit should keep returning bool not int to keep the chan
...
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1583255041)
Thanks ryanofsky, updated per feedback with a tiny diff:
Didn't move `InitShutdownState` from `AppInitBasicSetup` to `AppInit` and instead provided the `NodeContext` ref to `AppInitBasicSetup`.
Not sure if you have an strong opinion about this (shoot if you have it) but the rationale was to keep the same workflow as we have now so the bitcoin-qt related changes b0267f2 doesn't introduce another `InitShutdownState` call.
> I think AppInit should keep returning bool not int to keep the chan
...
📝 fanquake locked a pull request: "BLSCT SubAddress Pool"
(https://github.com/bitcoin/bitcoin/pull/27839)
This PR adds support for the use of a SubAddress pool and the generation of different receiving addresses.
`getnewaddress` RPC method pulls addresses from the pool and shows to the requester receiving addresses.
By default it generates addresses under account `0`.
It also introduces changes in `blast::PublicKey` to store the underlying point as a `MclG1Point` object, instead of a byte vector, since deserialisation of the vector is a expensive operation.
(https://github.com/bitcoin/bitcoin/pull/27839)
This PR adds support for the use of a SubAddress pool and the generation of different receiving addresses.
`getnewaddress` RPC method pulls addresses from the pool and shows to the requester receiving addresses.
By default it generates addresses under account `0`.
It also introduces changes in `blast::PublicKey` to store the underlying point as a `MclG1Point` object, instead of a byte vector, since deserialisation of the vector is a expensive operation.
💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1583272346)
note to self: after rebased onto BIP331, need to make sure rejected ephemeral anchor tx is put in `m_recent_rejects_reconsiderable` filter instead of `m_recent_rejects`
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1583272346)
note to self: after rebased onto BIP331, need to make sure rejected ephemeral anchor tx is put in `m_recent_rejects_reconsiderable` filter instead of `m_recent_rejects`