๐ TheCharlatan approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1776264127)
Re-ACK eaf915d61d470372e63f41f11d6a873c1936f73f
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1776264127)
Re-ACK eaf915d61d470372e63f41f11d6a873c1936f73f
๐ฌ eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850977742)
> going to a casino does not oblige the entire planet to have an indelible mark of fact that you went to a casino. Inscriptions do.
The thing is that inscriptions were not simply possible and begun to be used only when Casey released his software. The concept of inserting arbitrary data was already possible, and thus done, throughout bitcoin's history (this means your node already had arbitrary data, including various distasteful data). You can do a simple google search and see research papers
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850977742)
> going to a casino does not oblige the entire planet to have an indelible mark of fact that you went to a casino. Inscriptions do.
The thing is that inscriptions were not simply possible and begun to be used only when Casey released his software. The concept of inserting arbitrary data was already possible, and thus done, throughout bitcoin's history (this means your node already had arbitrary data, including various distasteful data). You can do a simple google search and see research papers
...
๐ฌ sinetek commented on issue "The logo icon doesn't show properly under Wayland":
(https://github.com/bitcoin-core/gui/issues/781#issuecomment-1850989318)
> a while ago, and I notice
it sohuld be simpler than that, something like QWindow::setIcon() ?
(https://github.com/bitcoin-core/gui/issues/781#issuecomment-1850989318)
> a while ago, and I notice
it sohuld be simpler than that, something like QWindow::setIcon() ?
๐ furszy approved a pull request: "tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places"
(https://github.com/bitcoin/bitcoin/pull/29055#pullrequestreview-1776302056)
ACK bd7f5d33
For a follow-up, we should make `CWallet::LoadWallet()` private and replace all those calls for `CWallet::Create()`.
(https://github.com/bitcoin/bitcoin/pull/29055#pullrequestreview-1776302056)
ACK bd7f5d33
For a follow-up, we should make `CWallet::LoadWallet()` private and replace all those calls for `CWallet::Create()`.
๐ mzumsande opened a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may of may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may of may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...
๐ mzumsande converted_to_draft a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may or may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...
(https://github.com/bitcoin/bitcoin/pull/29058)
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may or may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column
...
๐ฌ furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423225639)
> Ah oops my bad. I was backporting my proposed refactor from [5b8c165](https://github.com/bitcoin/bitcoin/commit/5b8c165c2ae0448b802c0c4907303d016f301f0a). I use a feerate of 3000 แนฉ/kvB there, but overlooked that it was still set to 0 here. Since the long-term feerate is set to 1000 แนฉ/vB here, this makes our transaction building follow high-feerate behavior.
>
> Iโve edited my code suggestion in the above comment to correct the feerate.
the approach is better but still fail. Knapsack retr
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423225639)
> Ah oops my bad. I was backporting my proposed refactor from [5b8c165](https://github.com/bitcoin/bitcoin/commit/5b8c165c2ae0448b802c0c4907303d016f301f0a). I use a feerate of 3000 แนฉ/kvB there, but overlooked that it was still set to 0 here. Since the long-term feerate is set to 1000 แนฉ/vB here, this makes our transaction building follow high-feerate behavior.
>
> Iโve edited my code suggestion in the above comment to correct the feerate.
the approach is better but still fail. Knapsack retr
...
๐ฌ furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851051245)
Updated per feedback. Thanks @murchandamus.
Test-only changes. Removed the assumption that BnB always executes first.
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851051245)
Updated per feedback. Thanks @murchandamus.
Test-only changes. Removed the assumption that BnB always executes first.
๐ค ajtowns reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307)
Okay, I think I'm on roughly the same page:
* keeping Split in util make sense
* moving our string stuff into the util namespace seems fine * moving the spanparsing stuff to script mostly makes sense -- though I feel a bit like mixing consensus critical script manipulation and miniscript/descriptor parsing in the same namespace is a bit clunky?
* moving fees.h/error.h to "common" makes sense; though if we can make "common" just be a broader part of "util" that seems better long term.
*
...
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307)
Okay, I think I'm on roughly the same page:
* keeping Split in util make sense
* moving our string stuff into the util namespace seems fine * moving the spanparsing stuff to script mostly makes sense -- though I feel a bit like mixing consensus critical script manipulation and miniscript/descriptor parsing in the same namespace is a bit clunky?
* moving fees.h/error.h to "common" makes sense; though if we can make "common" just be a broader part of "util" that seems better long term.
*
...
๐ฌ ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395)
Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395)
Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?
๐ฌ ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364)
It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?
`TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.
So putting this in common seems more sensible to me?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364)
It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?
`TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.
So putting this in common seems more sensible to me?
๐ค amitiuttarwar reviewed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1776309045)
code looks good to me. left a comment about the test that seems worth fixing, then I'm ready to ACK the patch.
trying to think about any potential negative network effects that could occur, but so far it seems fine to disconnect OB full relay peers that signal they don't want to receive transactions.
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1776309045)
code looks good to me. left a comment about the test that seems worth fixing, then I'm ready to ACK the patch.
trying to think about any potential negative network effects that could occur, but so far it seems fine to disconnect OB full relay peers that signal they don't want to receive transactions.
๐ฌ amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423210717)
```suggestion
self.log.info("Check that we sustain full-relay outbound connections to blocksonly peers when not at the full outbound limit")
```
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423210717)
```suggestion
self.log.info("Check that we sustain full-relay outbound connections to blocksonly peers when not at the full outbound limit")
```
๐ฌ amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423245744)
imo users should get an error if they set `-maxconnections` to less than 8, or at least a "this is unsafe. are you SURE?!". but that's probably getting off topic here :)
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423245744)
imo users should get an error if they set `-maxconnections` to less than 8, or at least a "this is unsafe. are you SURE?!". but that's probably getting off topic here :)
๐ฌ amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016)
I don't think this is testing the expected behavior. The test node is currently started up in `blocksonly` mode, so won't enter the new conditional in `EvictExtraOutboundPeers` regardless of the `extra_outbound_count` check. To get the test to fail, I added a restart. So right now, this 1st new test and the 3rd new test are the same thing.
My suggestion is to break these 3 new tests into a separate subtest that restarts with `-noblocksonly` to help make it clear what the preconditions are.
...
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016)
I don't think this is testing the expected behavior. The test node is currently started up in `blocksonly` mode, so won't enter the new conditional in `EvictExtraOutboundPeers` regardless of the `extra_outbound_count` check. To get the test to fail, I added a restart. So right now, this 1st new test and the 3rd new test are the same thing.
My suggestion is to break these 3 new tests into a separate subtest that restarts with `-noblocksonly` to help make it clear what the preconditions are.
...
๐ฌ amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423252977)
updating `ForEachNode` to no longer check `NodeFullyConnected` seems pretty intrusive & like all callers would need to be carefully checked, so imo it doesn't seem super valuable to repeat the check here. but cost of redundant check is also pretty low so doesn't seem like a big deal either way.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423252977)
updating `ForEachNode` to no longer check `NodeFullyConnected` seems pretty intrusive & like all callers would need to be carefully checked, so imo it doesn't seem super valuable to repeat the check here. but cost of redundant check is also pretty low so doesn't seem like a big deal either way.
๐ฌ amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423253383)
maybe `GetFullOutboundDelta` ?
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423253383)
maybe `GetFullOutboundDelta` ?
๐ฌ theStack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423278081)
nit: maybe '?' would also work to express the "detecting" state. But no hard feelings, typically users wouldn't see this a lot anyway.
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423278081)
nit: maybe '?' would also work to express the "detecting" state. But no hard feelings, typically users wouldn't see this a lot anyway.
๐ค theStack reviewed a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1776407268)
Concept ACK
Playing devil's advocate: could there be any noticeable drawbacks of always trying with v2 first? I'm thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don't follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that's the case, the user would only have the option to disable v2transport in general, or use the `addnode` RPC instead. Not sure how realistic that "i want to
...
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1776407268)
Concept ACK
Playing devil's advocate: could there be any noticeable drawbacks of always trying with v2 first? I'm thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don't follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that's the case, the user would only have the option to disable v2transport in general, or use the `addnode` RPC instead. Not sure how realistic that "i want to
...
๐ฌ theStack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423276132)
nit: can the comment above ("... does not currently have a way to signal BIP324 support") now also be removed? It's content is still true, but with the "always attempt v2 first if we support it"-approach, I guess it's not relevant anymore.
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423276132)
nit: can the comment above ("... does not currently have a way to signal BIP324 support") now also be removed? It's content is still true, but with the "always attempt v2 first if we support it"-approach, I guess it's not relevant anymore.