💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236)
Also, it just occurred to me, `GetLocalAddresses()` operates regardless of `-bind=` options. If the machine has two IPv6 addresses but `-bind=` has been used and only one of them specified, then we shouldn't do mapping for the other one (where nobody is listening).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236)
Also, it just occurred to me, `GetLocalAddresses()` operates regardless of `-bind=` options. If the machine has two IPv6 addresses but `-bind=` has been used and only one of them specified, then we shouldn't do mapping for the other one (where nobody is listening).
💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2197223467)
> * Applied suggestion in @theuni's [comment_1](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657337310) and [comment_2](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657354399), making the nonce used to initialize the caches a constructor argument, as well as re-using the same nonce for both caches. I did not expose it as an argument to the chainstate manager, but I'm also not completely sure if it is generated in a better spot now.
Paging @sipa for a quick concept
...
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2197223467)
> * Applied suggestion in @theuni's [comment_1](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657337310) and [comment_2](https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1657354399), making the nonce used to initialize the caches a constructor argument, as well as re-using the same nonce for both caches. I did not expose it as an argument to the chainstate manager, but I'm also not completely sure if it is generated in a better spot now.
Paging @sipa for a quick concept
...
🤔 ryanofsky reviewed a pull request: "logging: Use LogFatal instead LogError for fatal errors"
(https://github.com/bitcoin/bitcoin/pull/30347#pullrequestreview-2148219505)
Updated 475b70da309bbfd889968ce87d251f539f1d3351 -> b27dc672f6e575d739e037b6e46202e619bc386b ([`pr/logfat.1`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.1) -> [`pr/logfat.2`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/logfat.1..pr/logfat.2)) with suggested fixes and expanding it to cover `Logging::Warning` and `Logging::Error`.
---
re: https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490
...
(https://github.com/bitcoin/bitcoin/pull/30347#pullrequestreview-2148219505)
Updated 475b70da309bbfd889968ce87d251f539f1d3351 -> b27dc672f6e575d739e037b6e46202e619bc386b ([`pr/logfat.1`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.1) -> [`pr/logfat.2`](https://github.com/ryanofsky/bitcoin/commits/pr/logfat.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/logfat.1..pr/logfat.2)) with suggested fixes and expanding it to cover `Logging::Warning` and `Logging::Error`.
---
re: https://github.com/bitcoin/bitcoin/pull/30347#issuecomment-2194685490
...
💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862362)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039
I agree with you that the logging header should have better documentation, but I also think this section of developer notes is useful and provides good background information.
I actually already made some pretty significant improvements to documentation and organization of the logging header in #29256. If you would be interested in reviewing that PR (even just the documentation changes) I would really appreciate it. T
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862362)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657166039
I agree with you that the logging header should have better documentation, but I also think this section of developer notes is useful and provides good background information.
I actually already made some pretty significant improvements to documentation and organization of the logging header in #29256. If you would be interested in reviewing that PR (even just the documentation changes) I would really appreciate it. T
...
💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862683)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459
Agree this does create a small maintenence burden. FWIW, #29256 consolidates and [simplifies the macro definitions](https://github.com/ryanofsky/bitcoin/blob/681b21f7ca07f0e1411354baa01649e7b1f30e8c/src/logging.h#L327-L332) so the burden can get a littler smaller.
Overall though, I think the macros provide convenience and efficiency. I think we can support having both convenience and variable logging levels, and not
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862683)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657169459
Agree this does create a small maintenence burden. FWIW, #29256 consolidates and [simplifies the macro definitions](https://github.com/ryanofsky/bitcoin/blob/681b21f7ca07f0e1411354baa01649e7b1f30e8c/src/logging.h#L327-L332) so the burden can get a littler smaller.
Overall though, I think the macros provide convenience and efficiency. I think we can support having both convenience and variable logging levels, and not
...
💬 ryanofsky commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862564)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657237193
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot. Renaming Warning to Critical and Error to Fatal for the reasons given seems fine, but the old names shouldn't be kept around in that case.
I think the reality is we already have 7 levels of logging, but we are using 5 names to describe them. This PR is a small fix to make the names used in the code match the intent of the code.
Fo
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658862564)
re: https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657237193
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot. Renaming Warning to Critical and Error to Fatal for the reasons given seems fine, but the old names shouldn't be kept around in that case.
I think the reality is we already have 7 levels of logging, but we are using 5 names to describe them. This PR is a small fix to make the names used in the code match the intent of the code.
Fo
...
💬 mzumsande commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2197228936)
> Rearranging the order of the commits to make it easier to test the bugfix: the first test ([53f714d](https://github.com/bitcoin/bitcoin/commit/53f714d8bfd2331651445bcadb773a10f4d30264)) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix ([65343ec](https://github.com/bitcoin/bitcoin/commit/65343ec49a6b73c4197dfc38e1c2f433b0a3838a)).
I'd suggest to revert that: The CI should to pass on each commit, we even have a designated "test each commit" j
...
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2197228936)
> Rearranging the order of the commits to make it easier to test the bugfix: the first test ([53f714d](https://github.com/bitcoin/bitcoin/commit/53f714d8bfd2331651445bcadb773a10f4d30264)) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix ([65343ec](https://github.com/bitcoin/bitcoin/commit/65343ec49a6b73c4197dfc38e1c2f433b0a3838a)).
I'd suggest to revert that: The CI should to pass on each commit, we even have a designated "test each commit" j
...
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2197235201)
If there are further nits, I will address them in a follow-up.
Ready for more (re)review.
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2197235201)
If there are further nits, I will address them in a follow-up.
Ready for more (re)review.
💬 TheCharlatan commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197248760)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918
> I don't understand where this is coming from. Libraries can export globals, eg stdin, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.
Being forced to eliminate all globals doesn't seem to be the claim here? As long as the globals don't carry over annoying state from previous usage, require annoying initialization, or aren't mostly safe from user misuse, their u
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2197248760)
Re https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2194940918
> I don't understand where this is coming from. Libraries can export globals, eg stdin, so I don't see why you're claiming it's necessary for the kernel when it's not even necessary for libc.
Being forced to eliminate all globals doesn't seem to be the claim here? As long as the globals don't carry over annoying state from previous usage, require annoying initialization, or aren't mostly safe from user misuse, their u
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658972919)
Went ahead with 2 example types displayed.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658972919)
Went ahead with 2 example types displayed.
📝 Jianru-Lin opened a pull request: "Correct Error Code in OP_IF/OP_NOTIF Empty Stack Check"
(https://github.com/bitcoin/bitcoin/pull/30359)
### Description
This pull request addresses an error code inconsistency in the handling of empty stacks for the `OP_IF` and `OP_NOTIF` Bitcoin Script opcodes.
Currently, if the stack is empty when evaluating these opcodes, the script returns `SCRIPT_ERR_UNBALANCED_CONDITIONAL`, which is misleading. The correct error code for this scenario should be `SCRIPT_ERR_INVALID_STACK_OPERATION`, as the issue lies in insufficient stack elements rather than mismatched conditionals.
This change modi
...
(https://github.com/bitcoin/bitcoin/pull/30359)
### Description
This pull request addresses an error code inconsistency in the handling of empty stacks for the `OP_IF` and `OP_NOTIF` Bitcoin Script opcodes.
Currently, if the stack is empty when evaluating these opcodes, the script returns `SCRIPT_ERR_UNBALANCED_CONDITIONAL`, which is misleading. The correct error code for this scenario should be `SCRIPT_ERR_INVALID_STACK_OPERATION`, as the issue lies in insufficient stack elements rather than mismatched conditionals.
This change modi
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658977149)
Made this change.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1658977149)
Made this change.
💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658995029)
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot.
Meta:
It's not clear why people are 👍 on that comment. Are 5 "a lot", despite what other projects do, or based on our needs? Or is the 👍 about 5 being ok, but not having more than 5?
I've seen drive-by 👍 on other comments recently where the comment made multiple points, and it was similarly not clear: which of the points the 👍 pertains to? All? Some?
If a reviewer has a comment, I think it would
...
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1658995029)
> I don't think it makes sense for us to have 7 levels of logging -- even 5 is a lot.
Meta:
It's not clear why people are 👍 on that comment. Are 5 "a lot", despite what other projects do, or based on our needs? Or is the 👍 about 5 being ok, but not having more than 5?
I've seen drive-by 👍 on other comments recently where the comment made multiple points, and it was similarly not clear: which of the points the 👍 pertains to? All? Some?
If a reviewer has a comment, I think it would
...
💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2197273180)
After going through https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071, [sv2-spec](https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md), https://github.com/stratum-mining/sv2-spec/discussions/85 time and time again, I put all the relevant information in [sv2-draft-sketch.pdf](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf) for the Config A case, to make better sense of the trade offs involved.
With the assumption that I unde
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2197273180)
After going through https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071, [sv2-spec](https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md), https://github.com/stratum-mining/sv2-spec/discussions/85 time and time again, I put all the relevant information in [sv2-draft-sketch.pdf](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf) for the Config A case, to make better sense of the trade offs involved.
With the assumption that I unde
...
💬 jonatack commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164)
@ryanofsky
> A description of the actual log levels in our code base is
I agree. When I was working on #25203, for instance, it was often unclear to me whether to use error, warning, or info, and your classification would have made the choice clear. I am also not sure that we can usefully eliminate all helpful logging in one or more of the categories you described.
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659004164)
@ryanofsky
> A description of the actual log levels in our code base is
I agree. When I was working on #25203, for instance, it was often unclear to me whether to use error, warning, or info, and your classification would have made the choice clear. I am also not sure that we can usefully eliminate all helpful logging in one or more of the categories you described.
💬 cobratbq commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#discussion_r1659007178)
Done. I hope you like it ... it's become a bit convoluted, but it certainly is more descriptive.
(https://github.com/bitcoin/bitcoin/pull/30354#discussion_r1659007178)
Done. I hope you like it ... it's become a bit convoluted, but it certainly is more descriptive.
🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2148425344)
Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2148425344)
Fantastic change, would be helpful if you could make it a bit more digestible by separating it into smaller commits (especially splitting high-risk changes from low-risk refactoring), explaining the changes in commit messages.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659000088)
```suggestion
auto attr = entry.GetFlags();
```
(or just remove the assert, doesn't make a lot of sense to me)
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659000088)
```suggestion
auto attr = entry.GetFlags();
```
(or just remove the assert, doesn't make a lot of sense to me)
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659006186)
Yeah, too many changes in one commit
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659006186)
Yeah, too many changes in one commit
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658985403)
doing refactoring and logic changes in the same commit makes reviewing slightly harder - could you extract all non-critical changes in a separate commit so that we can concentrate on high-risk code more than on stylistic changes (which are important, but shouldn't distract us)
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658985403)
doing refactoring and logic changes in the same commit makes reviewing slightly harder - could you extract all non-critical changes in a separate commit so that we can concentrate on high-risk code more than on stylistic changes (which are important, but shouldn't distract us)