💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173930525)
Done on the latest push
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173930525)
Done on the latest push
💬 dergoegge commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1173938033)
Is that actually possible? Seems like all the validation interfaces including the zmq one are unregistered before chainman is destroyed?
So it can't be holding a reference to blockman that is actually destroyed, see:
https://github.com/bitcoin/bitcoin/blob/f3f5c9712670fc2c9fb3b57580146aa248a5d36f/src/init.cpp#L327-L343
Or are you just saying that having a reference to the node context has better lifetime guarantees because basically nothing outlives the node context?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1173938033)
Is that actually possible? Seems like all the validation interfaces including the zmq one are unregistered before chainman is destroyed?
So it can't be holding a reference to blockman that is actually destroyed, see:
https://github.com/bitcoin/bitcoin/blob/f3f5c9712670fc2c9fb3b57580146aa248a5d36f/src/init.cpp#L327-L343
Or are you just saying that having a reference to the node context has better lifetime guarantees because basically nothing outlives the node context?
💬 dergoegge commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1518037638)
Code review ACK bab12a6a029bed86ddcc15d42a236a855e277e74
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1518037638)
Code review ACK bab12a6a029bed86ddcc15d42a236a855e277e74
💬 dergoegge commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1518046601)
Code review ACK ed3159e0eb105d9f46659bd8f8b2db27d94841de
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1518046601)
Code review ACK ed3159e0eb105d9f46659bd8f8b2db27d94841de
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1173954405)
> Is that actually possible?
Likely that I am missing something in the logic. The way I see it each iteration of the for loop creates and destroys the `chainman` by calling `std::make_unique` again, while the `Shutdown` code is not called during each iteration.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1173954405)
> Is that actually possible?
Likely that I am missing something in the logic. The way I see it each iteration of the for loop creates and destroys the `chainman` by calling `std::make_unique` again, while the `Shutdown` code is not called during each iteration.
👍 TheCharlatan approved a pull request: "build: use latest config.{guess,sub} in depends"
(https://github.com/bitcoin/bitcoin/pull/27508#pullrequestreview-1396060103)
ACK 4a3f1db4ea5f90277cf7f57c051a2285e8b42468
I also checked the content from
```
wget 'https://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.guess;hb=HEAD' -O config.guess
wget 'https://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.sub;hb=HEAD' -O config.sub
```
Guix builds:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
fef9152d593aa1cb3243f37b8c069ef863f4e270962e17918cfa8
...
(https://github.com/bitcoin/bitcoin/pull/27508#pullrequestreview-1396060103)
ACK 4a3f1db4ea5f90277cf7f57c051a2285e8b42468
I also checked the content from
```
wget 'https://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.guess;hb=HEAD' -O config.guess
wget 'https://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.sub;hb=HEAD' -O config.sub
```
Guix builds:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
fef9152d593aa1cb3243f37b8c069ef863f4e270962e17918cfa8
...
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808)
> Seems kind of weird to change the CLI to use a "test-only" RPC in any case
@fanquake, @amitiuttarwar `-generate` CLI currently [uses](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/bitcoin-cli.cpp#L1178) a [hidden RPC](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/rpc/mining.cpp#L1055) (`generatetoaddress`) too.
i don't think a normal user would be interested in the new/tried table breakdown of addresses. so hop
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808)
> Seems kind of weird to change the CLI to use a "test-only" RPC in any case
@fanquake, @amitiuttarwar `-generate` CLI currently [uses](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/bitcoin-cli.cpp#L1178) a [hidden RPC](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/rpc/mining.cpp#L1055) (`generatetoaddress`) too.
i don't think a normal user would be interested in the new/tried table breakdown of addresses. so hop
...
📝 stratospher opened a pull request: "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count"
(https://github.com/bitcoin/bitcoin/pull/27511)
implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.
This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.
```jsx
$ getaddrmaninfo
Result:
{ (json object) json obj
...
(https://github.com/bitcoin/bitcoin/pull/27511)
implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.
This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.
```jsx
$ getaddrmaninfo
Result:
{ (json object) json obj
...
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173965885)
> Maybe borrow from the current -addrinfo help, along the lines of CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.
thanks @jonatack! this sounds much better.
> It might be interesting to have both with and without IsTerrible() filtering to maintain the current info returned and to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo u
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173965885)
> Maybe borrow from the current -addrinfo help, along the lines of CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.
thanks @jonatack! this sounds much better.
> It might be interesting to have both with and without IsTerrible() filtering to maintain the current info returned and to be able to compare over time, either by returning a breakdown in the new RPC or by leaving -addrinfo u
...
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799)
Getting stats on filtered/unfiltered addresses is interesting data!
But also feeling confused about what to do here - so haven't changed anything yet. (updated the PR to just discuss CLI changes, opened https://github.com/bitcoin/bitcoin/pull/27511 for getaddrmaninfo RPC)
1. what happens in the long run? In the long run, wouldn't ordinary users find it better if the CLI has a simple display of just breakdown of addresses by network? they wouldn't care much about filtering. I'd imagine we'
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799)
Getting stats on filtered/unfiltered addresses is interesting data!
But also feeling confused about what to do here - so haven't changed anything yet. (updated the PR to just discuss CLI changes, opened https://github.com/bitcoin/bitcoin/pull/27511 for getaddrmaninfo RPC)
1. what happens in the long run? In the long run, wouldn't ordinary users find it better if the CLI has a simple display of just breakdown of addresses by network? they wouldn't care much about filtering. I'd imagine we'
...
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173967154)
you're right! I've updated `getaddrmaninfo` taking both these suggestions.
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173967154)
you're right! I've updated `getaddrmaninfo` taking both these suggestions.
⚠️ dongcarl opened an issue: "user config: Support XDG Base Directory Specification or `$HOME/.{config,local}/`"
(https://github.com/bitcoin/bitcoin/issues/27512)
The [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html) has been around for a while and there seems to be growing support for it in various applications. This issue is to start discussion around **if** and **how** we should support it.
If the project is interested in supporting it (or a subset of it), here's an example of what we could do for the config file:
1. Check for config files in the following order (first hit is applied,
...
(https://github.com/bitcoin/bitcoin/issues/27512)
The [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html) has been around for a while and there seems to be growing support for it in various applications. This issue is to start discussion around **if** and **how** we should support it.
If the project is interested in supporting it (or a subset of it), here's an example of what we could do for the config file:
1. Check for config files in the following order (first hit is applied,
...
💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1518075524)
thank you for the useful feedback! Splitting this into 2 separate PRs since it’d be easier to think about RPC and CLI parts separately. I’ve opened https://github.com/bitcoin/bitcoin/pull/27511 for getaddrmaninfo RPC and updated this PR to reflect just the CLI changes.
- I liked returning objects as output and displaying total addresses per network ideas. Updated getaddrmaninfo in https://github.com/bitcoin/bitcoin/pull/27511 to use these.
- Haven't changed the previous CLI approach because
...
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1518075524)
thank you for the useful feedback! Splitting this into 2 separate PRs since it’d be easier to think about RPC and CLI parts separately. I’ve opened https://github.com/bitcoin/bitcoin/pull/27511 for getaddrmaninfo RPC and updated this PR to reflect just the CLI changes.
- I liked returning objects as output and displaying total addresses per network ideas. Updated getaddrmaninfo in https://github.com/bitcoin/bitcoin/pull/27511 to use these.
- Haven't changed the previous CLI approach because
...
💬 fanquake commented on issue "user config: Support XDG Base Directory Specification or `$HOME/.{config,local}/`":
(https://github.com/bitcoin/bitcoin/issues/27512#issuecomment-1518085815)
Dupe of #16733?
(https://github.com/bitcoin/bitcoin/issues/27512#issuecomment-1518085815)
Dupe of #16733?
💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173977053)
Please don't break user space. Again suggest https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618.
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173977053)
Please don't break user space. Again suggest https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1147473618.
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1518103852)
This PR is based on master from two months age. You may need to rebase it to current master for the CI to be green.
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1518103852)
This PR is based on master from two months age. You may need to rebase it to current master for the CI to be green.
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1518117272)
Tested Concept ACK. It might be good to have the breakdown of pre/post `IsTerrible` (RPC getnodeaddresses and CLI -addrinfo return post) -- see the large differences between them.
```
jon|(54a4f196190...):~/bitcoin/bitcoin$ ./src/bitcoin-cli -rpcwait getaddrmaninfo
{
"ipv4": {
"new": 29414,
"tried": 2043,
"total": 31457
},
"ipv6": {
"new": 6909,
"tried": 544,
"total": 7453
},
"onion": {
"new": 11499,
"tried": 5164,
"total": 16663
...
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1518117272)
Tested Concept ACK. It might be good to have the breakdown of pre/post `IsTerrible` (RPC getnodeaddresses and CLI -addrinfo return post) -- see the large differences between them.
```
jon|(54a4f196190...):~/bitcoin/bitcoin$ ./src/bitcoin-cli -rpcwait getaddrmaninfo
{
"ipv4": {
"new": 29414,
"tried": 2043,
"total": 31457
},
"ipv6": {
"new": 6909,
"tried": 544,
"total": 7453
},
"onion": {
"new": 11499,
"tried": 5164,
"total": 16663
...
💬 dongcarl commented on issue "user config: Support XDG Base Directory Specification or `$HOME/.{config,local}/`":
(https://github.com/bitcoin/bitcoin/issues/27512#issuecomment-1518117482)
True, will post what I wrote there and close.
(https://github.com/bitcoin/bitcoin/issues/27512#issuecomment-1518117482)
True, will post what I wrote there and close.
✅ dongcarl closed an issue: "user config: Support XDG Base Directory Specification or `$HOME/.{config,local}/`"
(https://github.com/bitcoin/bitcoin/issues/27512)
(https://github.com/bitcoin/bitcoin/issues/27512)
💬 dongcarl commented on issue "Comply with the XDG Base Directory Specification":
(https://github.com/bitcoin/bitcoin/issues/16733#issuecomment-1518118024)
Was thinking about this myself, here's an example of what we could do for the config file:
1. Check for config files in the following order (first hit is applied, warn if multiple found)
1. `-conf` command line parameter
1. `$datadir/bitcoin.conf`
1. `$XDG_CONFIG_HOME/.config/bitcoin/bitcoin.conf`
1. `$HOME/.config/bitcoin/bitcoin.conf`
1. `$HOME/.bitcoin/bitcoin/bitcoin.conf` (current non-XDG directory)
A question: should we use `$XDG_DATA_HOME` (`$HOME/.local/sha
...
(https://github.com/bitcoin/bitcoin/issues/16733#issuecomment-1518118024)
Was thinking about this myself, here's an example of what we could do for the config file:
1. Check for config files in the following order (first hit is applied, warn if multiple found)
1. `-conf` command line parameter
1. `$datadir/bitcoin.conf`
1. `$XDG_CONFIG_HOME/.config/bitcoin/bitcoin.conf`
1. `$HOME/.config/bitcoin/bitcoin.conf`
1. `$HOME/.bitcoin/bitcoin/bitcoin.conf` (current non-XDG directory)
A question: should we use `$XDG_DATA_HOME` (`$HOME/.local/sha
...