💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657757991)
3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132
Feels like this should be part of `Remove()`, which I think highlights some possible confusion between the two `class PrivateBroadcast` you have. One in ConnMan and one in its own unit, used in PeerManager.
I dunno if there's a cleaner approach available, to ensure that the two objects don't get out of sync? Like instead of `m_num_to_open` in ConnMan, could you refer to the size of the `ByNodeId` map?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657757991)
3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132
Feels like this should be part of `Remove()`, which I think highlights some possible confusion between the two `class PrivateBroadcast` you have. One in ConnMan and one in its own unit, used in PeerManager.
I dunno if there's a cleaner approach available, to ensure that the two objects don't get out of sync? Like instead of `m_num_to_open` in ConnMan, could you refer to the size of the `ByNodeId` map?
💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657739692)
3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132
This made me wonder, if the tx gets confirmed in a block before we get it via relay, will it ever get removed?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657739692)
3f5a4187a9e5d2bbb65c06c1799155fd7dd0d132
This made me wonder, if the tx gets confirmed in a block before we get it via relay, will it ever get removed?
💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657760369)
what about `m_by_nodeid`?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1657760369)
what about `m_by_nodeid`?
💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2195621640)
Now that the `IsMine` changes were moved to another PR, could reorder the commits just so 0cce574 is introduced before c3373a39a1acc94dbf644103fab2e477d49da671. This will let us keep the IsMine assertions inside `MigrateToDescriptor` for now.
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2195621640)
Now that the `IsMine` changes were moved to another PR, could reorder the commits just so 0cce574 is introduced before c3373a39a1acc94dbf644103fab2e477d49da671. This will let us keep the IsMine assertions inside `MigrateToDescriptor` for now.
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2195624752)
@maflcko @TheCharlatan
Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2195624752)
@maflcko @TheCharlatan
Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?
💬 hodlinator commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2195631997)
> On disallowing category arguments in `LogInfo`, that is an interesting line of reasoning that would not have occurred to me. Your suggestion to only accept source arguments not category arguments for `LogInfo` would probably be ok and is something I can implement.
> I do think there are plausible reasons to want to disallow attaching categories to certain log statements. If the only way I can make progress on this PR is to refuse to allow categories to be specified, I can make changes imple
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2195631997)
> On disallowing category arguments in `LogInfo`, that is an interesting line of reasoning that would not have occurred to me. Your suggestion to only accept source arguments not category arguments for `LogInfo` would probably be ok and is something I can implement.
> I do think there are plausible reasons to want to disallow attaching categories to certain log statements. If the only way I can make progress on this PR is to refuse to allow categories to be specified, I can make changes imple
...
👍 ryanofsky approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2146490664)
Code review ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2146490664)
Code review ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1657807539)
Maybe go more general to increase MacOS/darwin compatibility:
```
...
+ "\nLoad wallet using absolute path (Unix):\n"
+ HelpExampleCli("loadwallet", "\"/path/to/specialWallet/\"")
+ HelpExampleRpc("loadwallet", "\"/path/to/specialWallet/\"")
+ "\nLoad wallet using absolute path (Windows):\n"
+ HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
+ HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
...
```
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1657807539)
Maybe go more general to increase MacOS/darwin compatibility:
```
...
+ "\nLoad wallet using absolute path (Unix):\n"
+ HelpExampleCli("loadwallet", "\"/path/to/specialWallet/\"")
+ HelpExampleRpc("loadwallet", "\"/path/to/specialWallet/\"")
+ "\nLoad wallet using absolute path (Windows):\n"
+ HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
+ HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
...
```
👍 ryanofsky approved a pull request: "Mining interface followups, reduce cs_main locking, test rpc bug fix"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2146507019)
Code review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed. Just new error string is added since last review, and a commit message was updated
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2146507019)
Code review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed. Just new error string is added since last review, and a commit message was updated
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2195670730)
> Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?
Not sure what you mean, but this depends on your OS. You can install it normally. For example:
```
# apt install clang-16 && clang-16 --version
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
clang-16 is already the newest version (1:16.0.6-23ubuntu4).
0 upgraded, 0 newly installed, 0 to remove and 69 not upgraded.
Ubuntu clang ve
...
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2195670730)
> Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?
Not sure what you mean, but this depends on your OS. You can install it normally. For example:
```
# apt install clang-16 && clang-16 --version
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
clang-16 is already the newest version (1:16.0.6-23ubuntu4).
0 upgraded, 0 newly installed, 0 to remove and 69 not upgraded.
Ubuntu clang ve
...
👍 tdb3 approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2146552397)
cr ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2146552397)
cr ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe
🚀 ryanofsky merged a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167)
(https://github.com/bitcoin/bitcoin/pull/28167)
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2195702113)
> Let me know if I'm missing something but at what point is the fuzzed wallet funded? I generated a coverage report and the test seems to blocked at SelectCoins in CreateTransactionInternal. It always results in an insufficient funds error, which means the fuzzer never gets past this line in CreateTransaction.
It is not being funded, I will add it. Thanks.
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2195702113)
> Let me know if I'm missing something but at what point is the fuzzed wallet funded? I generated a coverage report and the test seems to blocked at SelectCoins in CreateTransactionInternal. It always results in an insufficient funds error, which means the fuzzer never gets past this line in CreateTransaction.
It is not being funded, I will add it. Thanks.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657850742)
I don't think there is a need for that. If `parent == child` then this is a no-op (every transaction is already an ancestor and descendant of itself).
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657850742)
I don't think there is a need for that. If `parent == child` then this is a no-op (every transaction is already an ancestor and descendant of itself).
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857264)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857264)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857307)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857307)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857391)
Added a comment.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857391)
Added a comment.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857455)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857455)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857579)
Done with a `static_assert`.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857579)
Done with a `static_assert`.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857631)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857631)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857760)
Done with a `static_assert`.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1657857760)
Done with a `static_assert`.