๐ฌ ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423078819)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388253872
> In `node::AbortNode()` though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense [there](https://github.com/bitcoin/bitcoin/pull/28051/files#diff-01b4a8b64504f4f4879e9a7fbac613d882a54975c9629c814e844463fb786a8fR19-R25)? (non-blocking nit, ofc)
Sorry I missed this comment before, and you are right about AbortNode. I renamed interrupt to shutdown there now and in the KernelN
...
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423078819)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1388253872
> In `node::AbortNode()` though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense [there](https://github.com/bitcoin/bitcoin/pull/28051/files#diff-01b4a8b64504f4f4879e9a7fbac613d882a54975c9629c814e844463fb786a8fR19-R25)? (non-blocking nit, ofc)
Sorry I missed this comment before, and you are right about AbortNode. I renamed interrupt to shutdown there now and in the KernelN
...
๐ฌ kashifs commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1850966201)
Hi @murchandamus,
I believe that I correctly opened a PR against this PR. Please let me know if this is the correct etiquette or if there is anything more that I should do.
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1850966201)
Hi @murchandamus,
I believe that I correctly opened a PR against this PR. Please let me know if this is the correct etiquette or if there is anything more that I should do.
๐ฌ ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850970368)
> One of the guiding principles for the kernel project so far has been to prohibit future re-couplings of decoupled modules.
I reread the issue you linked to, but I don't understand what it implies here. I do think you can rely on linker errors to ensure kernel code isn't calling wallet code or GUI code or P2P code or storing state in global variables. But what else do you want beyond that? And how does removing `sock.cpp` from the util library help, for example?
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850970368)
> One of the guiding principles for the kernel project so far has been to prohibit future re-couplings of decoupled modules.
I reread the issue you linked to, but I don't understand what it implies here. I do think you can rely on linker errors to ensure kernel code isn't calling wallet code or GUI code or P2P code or storing state in global variables. But what else do you want beyond that? And how does removing `sock.cpp` from the util library help, for example?
๐ 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` ?