💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977907287)
I posted the link above with example and I haven't seen a case where the RPC was hidden in there, so I think "the way it has always been" was to not deprecate. We haven't removed a whole RPC in a while though. But we can look at the options and returned values that were deprecated too: In that case it would make sense to remove the documentation of these options at the same time as we are deprecating them if we are following the justification for hiding. But instead we only add information that
...
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977907287)
I posted the link above with example and I haven't seen a case where the RPC was hidden in there, so I think "the way it has always been" was to not deprecate. We haven't removed a whole RPC in a while though. But we can look at the options and returned values that were deprecated too: In that case it would make sense to remove the documentation of these options at the same time as we are deprecating them if we are following the justification for hiding. But instead we only add information that
...
💬 jonatack commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977908894)
> We shouldn't have `|` at the end of the last clause, as this will make it match the empty string too (so effectively everything starting with `Satoshi:` matches).
Good catch. It looks like this would mean that the regex has been doing this since a8ec9eede4c745c6b6fd76974816ffad8034129a one year ago?
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977908894)
> We shouldn't have `|` at the end of the last clause, as this will make it match the empty string too (so effectively everything starting with `Satoshi:` matches).
Good catch. It looks like this would mean that the regex has been doing this since a8ec9eede4c745c6b6fd76974816ffad8034129a one year ago?
💬 jonatack commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977921907)
I don't recall deprecated RPCs being set to hidden in the past. Generally we do that for developer-only ones that could represent footguns for users. If this RPC is considered to be sufficiently dangerous, that could perhaps justify hiding it, but not due only to being deprecated.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977921907)
I don't recall deprecated RPCs being set to hidden in the past. Generally we do that for developer-only ones that could represent footguns for users. If this RPC is considered to be sufficiently dangerous, that could perhaps justify hiding it, but not due only to being deprecated.
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2695119699)
@dergoegge
I agree and I'd like to add those tests in a second PR if this PR is merged?
I don't have a lot of time this month and next to work on them and I worry about putting in more time for a PR that may not be merged.
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2695119699)
@dergoegge
I agree and I'd like to add those tests in a second PR if this PR is merged?
I don't have a lot of time this month and next to work on them and I worry about putting in more time for a PR that may not be merged.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1977960657)
I may have a situation in HTTP where SendBytes() is being called from a thread other than the Sockman I/O loop, which would be an "optimistic send" directly from a worker thread. `CConnman::PushMessage()` has similar logic, but since p2p doesn't use worker threads I don't think it would cause an issue there. I think this assertion may be the only real conflict for that. `m_connected_mutex` is only used in `GetConnectionSockets()` -- would a little lock-waiting there be so terrible?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1977960657)
I may have a situation in HTTP where SendBytes() is being called from a thread other than the Sockman I/O loop, which would be an "optimistic send" directly from a worker thread. `CConnman::PushMessage()` has similar logic, but since p2p doesn't use worker threads I don't think it would cause an issue there. I think this assertion may be the only real conflict for that. `m_connected_mutex` is only used in `GetConnectionSockets()` -- would a little lock-waiting there be so terrible?
💬 laanwj commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977961350)
Yes, i didn't notice before ! It's too easy to make this mistake.
It also makes me wonder whether this is a useful filter at all, or whether to replace it with a simpler one that extracts the version number, followed by some minimum/maximum bounds check. But that's not something that needs to be addressed in this PR.
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977961350)
Yes, i didn't notice before ! It's too easy to make this mistake.
It also makes me wonder whether this is a useful filter at all, or whether to replace it with a simpler one that extracts the version number, followed by some minimum/maximum bounds check. But that's not something that needs to be addressed in this PR.
💬 jonatack commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977962385)
Not sure I see a difference in the makeseeds output. Done in the first commit attributed to you (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977962385)
Not sure I see a difference in the makeseeds output. Done in the first commit attributed to you (thanks!)
💬 laanwj commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977969707)
Thanks!
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1977969707)
Thanks!
💬 jonatack commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2695206796)
Updated to take @laanwj's review feedback and re-ran the seeds scripts.
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2695206796)
Updated to take @laanwj's review feedback and re-ran the seeds scripts.
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977976554)
My thinking of deprecating a feature is discouraging its use without outright disabling it, only allowing it when explicitly required.
Hiding it from the help text is a good step in this direction.
You can still access details about the RPC by running:
bitcoin-cli settxfee --help
You just won’t see the feature description in the overview of available RPCs.
As for paytxfee, it is still listed in the startup option help text.
AFAICT, it is not deprecated it's just warnings th
...
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1977976554)
My thinking of deprecating a feature is discouraging its use without outright disabling it, only allowing it when explicitly required.
Hiding it from the help text is a good step in this direction.
You can still access details about the RPC by running:
bitcoin-cli settxfee --help
You just won’t see the feature description in the overview of available RPCs.
As for paytxfee, it is still listed in the startup option help text.
AFAICT, it is not deprecated it's just warnings th
...
💬 jonatack commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978004417)
Software clients that use the deprecated RPC API will see breakage when they update and look for why. It may be more confusing to no longer see the RPC in the list and potentially think it has been removed outright ("core devs broke my app without warning") rather than deprecated behind a flag. Hopefully, people read the release notes but 🤷.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978004417)
Software clients that use the deprecated RPC API will see breakage when they update and look for why. It may be more confusing to no longer see the RPC in the list and potentially think it has been removed outright ("core devs broke my app without warning") rather than deprecated behind a flag. Hopefully, people read the release notes but 🤷.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2695256954)
Rebased the previous conflict and #30901 has been merged now as well 🥳
I also added the `-DWITH_EMBEDDED_ASMAP=NO` to one of the CI jobs.
> would it make sense to provide the ability for a user (who doesn't necessarily have easy access to the source code) to extract the asmap data from a running node?
Let me address that in a follow-up. I think it's a nice-to-have since the necessary data for diffing should always be available in https://github.com/asmap/asmap-data when we follow the
...
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2695256954)
Rebased the previous conflict and #30901 has been merged now as well 🥳
I also added the `-DWITH_EMBEDDED_ASMAP=NO` to one of the CI jobs.
> would it make sense to provide the ability for a user (who doesn't necessarily have easy access to the source code) to extract the asmap data from a running node?
Let me address that in a follow-up. I think it's a nice-to-have since the necessary data for diffing should always be available in https://github.com/asmap/asmap-data when we follow the
...
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978010680)
fair point cc @polespinasa
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978010680)
fair point cc @polespinasa
💬 laanwj commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2695280949)
Code review ACK b2dc4458ac86c0a633ca3eb87827ac2731993f97 on the script changes
mainnet
```
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
IPv4 IPv6 Onion I2P CJDNS Pass
17173 3638 21303 3116 12 Initial
17173 3638 21303 3116 12 Skip entries with invalid address
8378 1755 14661 2343 10 After removing duplicates
8337 1736 14656 2343 10 Enforce minimal number of blocks
7986 1612 14654 2343
...
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2695280949)
Code review ACK b2dc4458ac86c0a633ca3eb87827ac2731993f97 on the script changes
mainnet
```
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
IPv4 IPv6 Onion I2P CJDNS Pass
17173 3638 21303 3116 12 Initial
17173 3638 21303 3116 12 Skip entries with invalid address
8378 1755 14661 2343 10 After removing duplicates
8337 1736 14656 2343 10 Enforce minimal number of blocks
7986 1612 14654 2343
...
💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978024595)
> It may be more confusing to no longer see the RPC in the list and potentially think it has been removed.
That's a fair point and a good reason to not hide it. I will just revert that.
> It would be good to have clear guidance on this.
Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978024595)
> It may be more confusing to no longer see the RPC in the list and potentially think it has been removed.
That's a fair point and a good reason to not hide it. I will just revert that.
> It would be good to have clear guidance on this.
Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.
📝 glozow opened a pull request: "delete release note fragments for v29"
(https://github.com/bitcoin/bitcoin/pull/31976)
Delete release note fragments in preparation for 29.x branch-off.
Everything here has been copied over to the draft release notes at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
(https://github.com/bitcoin/bitcoin/pull/31976)
Delete release note fragments in preparation for 29.x branch-off.
Everything here has been copied over to the draft release notes at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978034199)
> Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.
Good idea for a follow up PR, a section on removing a feature will be nice.
But we have different feature category.
1. RPC
2. Startup option
3. Rest
4. ZMQ interface
5. Wallet settings
6. In the future ( multiprocess interface)
We should document how we remove each.
Documenting for RPC is a good first step.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978034199)
> Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.
Good idea for a follow up PR, a section on removing a feature will be nice.
But we have different feature category.
1. RPC
2. Startup option
3. Rest
4. ZMQ interface
5. Wallet settings
6. In the future ( multiprocess interface)
We should document how we remove each.
Documenting for RPC is a good first step.
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978035081)
You should remove the old version of the release notes from the first commit too.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978035081)
You should remove the old version of the release notes from the first commit too.
💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695314625)
There were some opinions about the release notes
> I left comments there but I don't want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don't want to address them.
> (I didn't study the release notes again, but those can be changed later anyway)
> Further editing of the release notes can be done as part of the actual release process.
> It's explained in BlockAssembler::Options ClampOptions, though we should probably point to that fr
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695314625)
There were some opinions about the release notes
> I left comments there but I don't want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don't want to address them.
> (I didn't study the release notes again, but those can be changed later anyway)
> Further editing of the release notes can be done as part of the actual release process.
> It's explained in BlockAssembler::Options ClampOptions, though we should probably point to that fr
...
💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978049278)
Sorry, what do you mean? There's only one commit.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978049278)
Sorry, what do you mean? There's only one commit.
💬 glozow commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2695318085)
Unsure if a release note was written, apologies if I missed it. @hebasto @theuni @fanquake could you add to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft?
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2695318085)
Unsure if a release note was written, apologies if I missed it. @hebasto @theuni @fanquake could you add to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft?