💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646473144)
This seems incorrect, we clamp the number of elements, not the number of bytes. I'd prefer not documenting the clamping behaviour here, given how generous the limit is.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646473144)
This seems incorrect, we clamp the number of elements, not the number of bytes. I'd prefer not documenting the clamping behaviour here, given how generous the limit is.
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657671482)
nit: \*ducks\* it seems like now would be the perfect time to rename this to `SignatureCache`?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657671482)
nit: \*ducks\* it seems like now would be the perfect time to rename this to `SignatureCache`?
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657442012)
reviewer note: orthogonal to this move-only change, but this has all kinds of under- and overflow issues. Since it's a debug issue, i suppose it's not the end of the world though
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657442012)
reviewer note: orthogonal to this move-only change, but this has all kinds of under- and overflow issues. Since it's a debug issue, i suppose it's not the end of the world though
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657682792)
nit: any reason this isn't initialized where it's used?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657682792)
nit: any reason this isn't initialized where it's used?
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657457261)
I didn't realize the mutex has the same practical effect when I mentioned it it in my original [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)) - but I too would still prefer to be more explicit here. If for some reason that's not a good idea, can we at least add a comment to the mutex to indicate that it also serves this purpose in case it is updated in the future?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657457261)
I didn't realize the mutex has the same practical effect when I mentioned it it in my original [comment](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)) - but I too would still prefer to be more explicit here. If for some reason that's not a good idea, can we at least add a comment to the mutex to indicate that it also serves this purpose in case it is updated in the future?
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646343915)
nit: `std::optional` no longer needed as an include
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1646343915)
nit: `std::optional` no longer needed as an include
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195528186)
> Also, potential improvements like #28945 will make up for it.
It seems https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953 was abandoned since
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195528186)
> Also, potential improvements like #28945 will make up for it.
It seems https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953 was abandoned since
💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195529824)
> an extra 13 characters of noise each time (20 if you don't do something to avoid having to type `BCLog::Level::Debug`)
Extra complexity and code and an inconsistent API to avoid "extra characters" (that moreover many developers likely just cut and paste as needed) seems a poor (and borderline silly?) trade-off unless forced on us for technical reasons. Far better to have the code and API as simple and consistent as possible.
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195529824)
> an extra 13 characters of noise each time (20 if you don't do something to avoid having to type `BCLog::Level::Debug`)
Extra complexity and code and an inconsistent API to avoid "extra characters" (that moreover many developers likely just cut and paste as needed) seems a poor (and borderline silly?) trade-off unless forced on us for technical reasons. Far better to have the code and API as simple and consistent as possible.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195533792)
> > A non-pruned IBD with full dbcache to tip ended up using 12% more memory, but it was also 2.7% faster somehow.
>
> I'm not sure if I understand this, is this still the case? Why would a node use more memory after this pull? Shouldn't the 2 added pointers be included in the dbcache accounting, resulting in more frequent flushes due to the cache filling up faster, but not in an increase in memory when the cache is full?
This is no longer the case. When I did the initial benchmarks the fu
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195533792)
> > A non-pruned IBD with full dbcache to tip ended up using 12% more memory, but it was also 2.7% faster somehow.
>
> I'm not sure if I understand this, is this still the case? Why would a node use more memory after this pull? Shouldn't the 2 added pointers be included in the dbcache accounting, resulting in more frequent flushes due to the cache filling up faster, but not in an increase in memory when the cache is full?
This is no longer the case. When I did the initial benchmarks the fu
...
💬 jonatack commented on issue "RFC: Misused LogError and LogWarning macros":
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195538575)
Two years ago, it seemed we had a road map on severity-level logging based on a PR discussion. It turned out not to be the case, and not sufficiently specified, with subsequent proposals to swerve here or take another road there.
Not sure, but perhaps it would be best to gather a larger consensus on which log levels to adopt, and with which definitions, and an overall API, rather than everyone opening competing proposals. But I haven't been following all of these proposals closely.
(https://github.com/bitcoin/bitcoin/issues/30348#issuecomment-2195538575)
Two years ago, it seemed we had a road map on severity-level logging based on a PR discussion. It turned out not to be the case, and not sufficiently specified, with subsequent proposals to swerve here or take another road there.
Not sure, but perhaps it would be best to gather a larger consensus on which log levels to adopt, and with which definitions, and an overall API, rather than everyone opening competing proposals. But I haven't been following all of these proposals closely.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195540246)
> > Also, potential improvements like #28945 will make up for it.
>
> It seems [#28945 (comment)](https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953) was abandoned since
In my recent benchmarks it was again slightly faster on this branch for default dbcache and no pruning. So I think this PR is essentially the same.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2195540246)
> > Also, potential improvements like #28945 will make up for it.
>
> It seems [#28945 (comment)](https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-2081170953) was abandoned since
In my recent benchmarks it was again slightly faster on this branch for default dbcache and no pruning. So I think this PR is essentially the same.
📝 furszy opened a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353)
Fix https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1657628378 inconsistency.
Block 'fundrawtransaction' from automatically picking up inputs when testing the funding of a transaction using only pre-selected inputs.
(https://github.com/bitcoin/bitcoin/pull/30353)
Fix https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1657628378 inconsistency.
Block 'fundrawtransaction' from automatically picking up inputs when testing the funding of a transaction using only pre-selected inputs.
💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1657761131)
I think calling `IsLocked()` would be more precise and make more sense then. It seems confusing to say that the wallet can still be encrypted but expect no encryption keys (we understand it because we know the loading procedure but others will definitely not understand it).
Other than that, what if instead of creating this new `LegacyDataSPKM::AddKeyPubKeyInner` method you place this three lines inside the `LegacyDataSPKM::LoadKey` which is only called during load?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1657761131)
I think calling `IsLocked()` would be more precise and make more sense then. It seems confusing to say that the wallet can still be encrypted but expect no encryption keys (we understand it because we know the loading procedure but others will definitely not understand it).
Other than that, what if instead of creating this new `LegacyDataSPKM::AddKeyPubKeyInner` method you place this three lines inside the `LegacyDataSPKM::LoadKey` which is only called during load?
💬 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\\\"")
...
```