💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225867669)
I understand the rationale, but it seems dangerous to skip validation steps in special cases because of expected checks happening later in another scope (that's how I understand it). Such things can lead to nasty errors down the line.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225867669)
I understand the rationale, but it seems dangerous to skip validation steps in special cases because of expected checks happening later in another scope (that's how I understand it). Such things can lead to nasty errors down the line.
💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1219838215)
How about actually checking the result here?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1219838215)
How about actually checking the result here?
💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225870352)
Kind of confusing and `grep` unfriendly that this test helper has the exact same name as that `Package` method. Maybe call it `TestAncestorPackage` or so?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225870352)
Kind of confusing and `grep` unfriendly that this test helper has the exact same name as that `Package` method. Maybe call it `TestAncestorPackage` or so?
💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225898875)
Just checking the last tx doesn't guarantee this alone. Maybe mention here that this works because subpackages are being evaluated by themselves as well?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225898875)
Just checking the last tx doesn't guarantee this alone. Maybe mention here that this works because subpackages are being evaluated by themselves as well?
💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225871000)
nit: Developer notes say "Class member variables have a `m_` prefix.".
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225871000)
nit: Developer notes say "Class member variables have a `m_` prefix.".
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913119)
latest push collects stats as a `CConnman` member, so we don't have to worry about iterating through all nodes
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913119)
latest push collects stats as a `CConnman` member, so we don't have to worry about iterating through all nodes
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913181)
updated to incorporate your suggestions, thanks!
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913181)
updated to incorporate your suggestions, thanks!
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913245)
moved to member to prevent iterating through all nodes
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913245)
moved to member to prevent iterating through all nodes
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913270)
done, thanks!
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913270)
done, thanks!
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913626)
we have now removed any clearnet bundling, so this quirk is no longer relevant
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913626)
we have now removed any clearnet bundling, so this quirk is no longer relevant
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225914692)
question: I noticed you used array in your implementation but we were using vector. is there a strong reason for that?
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225914692)
question: I noticed you used array in your implementation but we were using vector. is there a strong reason for that?
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1586321519)
thank you for the reviews @ajtowns & @vasild 🙌
significant changes with latest push:
* introduce a new member on `CConnman` called `m_network_conn_counts` which is a map from `Network` to an atomic that keeps track of the number of `OUTBOUND_FULL_RELAY` and `MANUAL` connections on the node broken down by network. this means we don't have to iterate through all nodes anymore.
* changed the interval from 1 min to 5 min.
* got rid of any "clearnet" bundling, now we treat IPV4 & IPV6 as s
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1586321519)
thank you for the reviews @ajtowns & @vasild 🙌
significant changes with latest push:
* introduce a new member on `CConnman` called `m_network_conn_counts` which is a map from `Network` to an atomic that keeps track of the number of `OUTBOUND_FULL_RELAY` and `MANUAL` connections on the node broken down by network. this means we don't have to iterate through all nodes anymore.
* changed the interval from 1 min to 5 min.
* got rid of any "clearnet" bundling, now we treat IPV4 & IPV6 as s
...
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225915157)
updated to have a small connman member to keep track of counts. now we don't iterate through all peers anymore :)
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225915157)
updated to have a small connman member to keep track of counts. now we don't iterate through all peers anymore :)
💬 Giszmo commented on issue "Indefinite "Bitcoin Core is shutting down..."":
(https://github.com/bitcoin/bitcoin/issues/27848#issuecomment-1586325943)
So I killed it, started it, waited to catch up and now Exit took some 5 or so seconds.
I suspect that the issue might have to do with flaky internet.
(https://github.com/bitcoin/bitcoin/issues/27848#issuecomment-1586325943)
So I killed it, started it, waited to catch up and now Exit took some 5 or so seconds.
I suspect that the issue might have to do with flaky internet.
💬 LarryRuane commented on pull request "test: allow BITCOIN_TEST_PATH to specify working dir":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1586362907)
Rebased for merge conflict.
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1586362907)
Rebased for merge conflict.
💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586454719)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586454719)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
💬 ajtowns commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586490389)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586490389)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
💬 ajtowns commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1586532229)
> > without #26711 we are still letting things into mempool that are possibly problematic, no?
>
> You're right. Every time we think about it a bit longer, another not-quite-ideal edge case comes up. Especially with full mempools and eviction being something to consider.
I think this should have remained as a separate PR, and should have been okay to merge as-is, rather than adding only loosely-related commits to #26711. Two reasons:
- after #26933 this doesn't allow any problematic be
...
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1586532229)
> > without #26711 we are still letting things into mempool that are possibly problematic, no?
>
> You're right. Every time we think about it a bit longer, another not-quite-ideal edge case comes up. Especially with full mempools and eviction being something to consider.
I think this should have remained as a separate PR, and should have been okay to merge as-is, rather than adding only loosely-related commits to #26711. Two reasons:
- after #26933 this doesn't allow any problematic be
...
💬 ajtowns commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226035519)
Should probably mark it as EXPERIMENTAL in the rpc docs -- see "sendall" eg in wallet/rpc/spend.cpp
Not really sure this should be advertised in the release notes at this stage at all, though.
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226035519)
Should probably mark it as EXPERIMENTAL in the rpc docs -- see "sendall" eg in wallet/rpc/spend.cpp
Not really sure this should be advertised in the release notes at this stage at all, though.
💬 ajtowns commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226063546)
This check doesn't feel like it makes sense to me -- it should either be unnecessary (because dealing with subpackages first takes care of it), or else it seems insufficient (because a grandparent might be paying for parent and child, and child paying for parent; but grandparents alone are good enough, but after they're accepted parent and child combined aren't good enough).
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226063546)
This check doesn't feel like it makes sense to me -- it should either be unnecessary (because dealing with subpackages first takes care of it), or else it seems insufficient (because a grandparent might be paying for parent and child, and child paying for parent; but grandparents alone are good enough, but after they're accepted parent and child combined aren't good enough).