💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612276973)
Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612276581.
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612276973)
Same comment here as https://github.com/bitcoin/bitcoin/pull/30062/files#r1612276581.
💬 maflcko commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2127983803)
I presume macos-cross compilation on risv64 metal still fails? If it is expected to pass now, I can re-try this.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2127983803)
I presume macos-cross compilation on risv64 metal still fails? If it is expected to pass now, I can re-try this.
💬 jonatack commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612279756)
Here and in the other new help below, the reason for the optional nature of this field could perhaps be mentioned (as simply as this ", if present", or perhaps in more detail).
```suggestion
{RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap, if present"},
```
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612279756)
Here and in the other new help below, the reason for the optional nature of this field could perhaps be mentioned (as simply as this ", if present", or perhaps in more detail).
```suggestion
{RPCResult::Type::NUM, "mapped_as", /*optional=*/true, "The ASN mapped to the IP of this peer per our current ASMap, if present"},
```
💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2128007110)
Your patch is much better, @theStack, thanks! It's simpler and doesn't need that weird `sleep`. Force-pushed your patch. If it passes CI, I think this is the way to go. I had tried something similar, but I didn't think to make the sending of that extra (smaller) transaction conditional based on available mempool space.
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2128007110)
Your patch is much better, @theStack, thanks! It's simpler and doesn't need that weird `sleep`. Force-pushed your patch. If it passes CI, I think this is the way to go. I had tried something similar, but I didn't think to make the sending of that extra (smaller) transaction conditional based on available mempool space.
💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612300529)
Makes sense, I will address it in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612300529)
Makes sense, I will address it in a follow-up PR.
💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2128020594)
Thanks for reviewing, @jonatack. I'm working on a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2128020594)
Thanks for reviewing, @jonatack. I'm working on a follow-up.
💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1612315058)
> Note that for transient I2P sessions (when `-i2pacceptincoming=0`) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
Good point. At the same time, I2P connections are intended to be long-running ones, as the tunnels are one-way and more time/resource intensive to launch than other networks. I'm unsure whether it's best for this to be verbose, or to set it as a less-frequent log level if `i2pacceptincoming` is off.
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1612315058)
> Note that for transient I2P sessions (when `-i2pacceptincoming=0`) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
Good point. At the same time, I2P connections are intended to be long-running ones, as the tunnels are one-way and more time/resource intensive to launch than other networks. I'm unsure whether it's best for this to be verbose, or to set it as a less-frequent log level if `i2pacceptincoming` is off.
💬 jonatack commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1612322539)
Concept ACK on additional checks.
Could this have been an individual python lint script like the other ones in `/test/lint/`? I appreciated being able to run individual python scripts locally to check my work or when reviewing. It seems we have been losing that recently.
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1612322539)
Concept ACK on additional checks.
Could this have been an individual python lint script like the other ones in `/test/lint/`? I appreciated being able to run individual python scripts locally to check my work or when reviewing. It seems we have been losing that recently.
👍 theStack approved a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2075196742)
re-ACK 2fd34ba504957331f5a08614b6e1f8317260f04d
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2075196742)
re-ACK 2fd34ba504957331f5a08614b6e1f8317260f04d
💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2128122156)
I would squash the first and last commits, as the latter is just retouching 2 lines already changed in the first commit, to `i2p: log with LogPrintLevel per severity level`
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2128122156)
I would squash the first and last commits, as the latter is just retouching 2 lines already changed in the first commit, to `i2p: log with LogPrintLevel per severity level`
📝 hebasto opened a pull request: "[PoC] ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164)
This PR add FreeBSD CI job based on https://github.com/vmactions.
Other *BSD OSes are also available.
Looking for Concept (N)ACKs.
IMPORTANT. The `vmactions/*` needs to be added to the "Actions permissions" section of the repository settings.
A CI job example in my personal repo -- https://github.com/hebasto/bitcoin/actions/runs/9215593397/job/25354268473.
(https://github.com/bitcoin/bitcoin/pull/30164)
This PR add FreeBSD CI job based on https://github.com/vmactions.
Other *BSD OSes are also available.
Looking for Concept (N)ACKs.
IMPORTANT. The `vmactions/*` needs to be added to the "Actions permissions" section of the repository settings.
A CI job example in my personal repo -- https://github.com/hebasto/bitcoin/actions/runs/9215593397/job/25354268473.
🤔 stickies-v reviewed a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2074818707)
Thank you for the review, @TheCharlatan and @ryanofsky .
In this force push, I've:
- rebased to address merge conflict from #30115
- incorporated @TheCharlatan's [suggestion](https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1610597554) to use `enum class` instead of `std::string` for the warning identifiers
- addressed @ryanofsky's comments:
- improved commit messages to describe the introduced behaviour change and improved a couple of docstrings
- added release notes
...
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2074818707)
Thank you for the review, @TheCharlatan and @ryanofsky .
In this force push, I've:
- rebased to address merge conflict from #30115
- incorporated @TheCharlatan's [suggestion](https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1610597554) to use `enum class` instead of `std::string` for the warning identifiers
- addressed @ryanofsky's comments:
- improved commit messages to describe the introduced behaviour change and improved a couple of docstrings
- added release notes
...
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612278118)
I've incorporated the id-as-enum approach in the latest force-push, thank you very much for both of your extensive feedback.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612278118)
I've incorporated the id-as-enum approach in the latest force-push, thank you very much for both of your extensive feedback.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612215147)
Sounds good, I've added release notes to cover that. The way I read the startup option docstring _("Execute command when an alert is raised (%s in cmd is replaced by message)")_, I think the new behaviour aligns better with how I'd expect this option to behave.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612215147)
Sounds good, I've added release notes to cover that. The way I read the startup option docstring _("Execute command when an alert is raised (%s in cmd is replaced by message)")_, I think the new behaviour aligns better with how I'd expect this option to behave.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612240542)
> I guess this change also affects the order of warnings.
It does indeed. I hadn't really considered this, and I think I agree that the current behaviour is fine - but I'll think about it more and open to suggestions.
> Changing this is probably fine, but it would be good to note the change in the commit message.
I've updated the commit message section on behaviour change to:
<details>
```
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifi
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612240542)
> I guess this change also affects the order of warnings.
It does indeed. I hadn't really considered this, and I think I agree that the current behaviour is fine - but I'll think about it more and open to suggestions.
> Changing this is probably fine, but it would be good to note the change in the commit message.
I've updated the commit message section on behaviour change to:
<details>
```
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifi
...
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612216154)
Ah makes sense, I've updated that, thanks.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612216154)
Ah makes sense, I've updated that, thanks.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612194682)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612194682)
Thanks, fixed.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612228366)
> Can commit message be updated to say what this change in behavior here is?
I've updated the commit message:
<details>
```
node: update uiInterface whenever warnings updated
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612228366)
> Can commit message be updated to say what this change in behavior here is?
I've updated the commit message:
<details>
```
node: update uiInterface whenever warnings updated
This commit introduces slight behaviour change. Previously, the
GUI status bar would be updated for most warnings, namely
UNKNOWN_NEW_RULES_ACTIVATED, CLOCK_OUT_OF_SYNC and
PRE_RELEASE_TEST_BUILD, but not for LARGE_WORK_INVALID_CHAIN
(and not for FATAL_INTERNAL_ERROR, but that is not really
meaningful).
...
🤔 jonatack reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2075246052)
Concept ACK, [`a0be62c` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/a0be62c9043caaebf64b3178b9bea080fa200ec7#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R543) contains these changes and further ones, perhaps look if other ones there could be done here.
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2075246052)
Concept ACK, [`a0be62c` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/a0be62c9043caaebf64b3178b9bea080fa200ec7#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R543) contains these changes and further ones, perhaps look if other ones there could be done here.
💬 jonatack commented on pull request "net: log connections failures via SOCKS5 with less severity":
(https://github.com/bitcoin/bitcoin/pull/30064#discussion_r1612411821)
In this case, I had added `Manual` and considered it an error (or perhaps warning), what do you think: https://github.com/bitcoin/bitcoin/pull/25203/commits/a0be62c9043caaebf64b3178b9bea080fa200ec7#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R541
```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Manual %s\n", error_message);
```
(https://github.com/bitcoin/bitcoin/pull/30064#discussion_r1612411821)
In this case, I had added `Manual` and considered it an error (or perhaps warning), what do you think: https://github.com/bitcoin/bitcoin/pull/25203/commits/a0be62c9043caaebf64b3178b9bea080fa200ec7#diff-3499e52d708f04ebd0bfeec799dd26464ca6bd26a802c700460227c4f41ec4b5R541
```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Manual %s\n", error_message);
```