🚀 fanquake merged a pull request: "[28.x] 28.2rc1"
(https://github.com/bitcoin/bitcoin/pull/32480)
(https://github.com/bitcoin/bitcoin/pull/32480)
💬 earonesty commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2887275676)
makes more sense to change the default to be larger or whatever not remove the option at all
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2887275676)
makes more sense to change the default to be larger or whatever not remove the option at all
💬 pinheadmz commented on issue ""rpcallowip=" configuration directive doesn't accept RFC4193 addresses":
(https://github.com/bitcoin/bitcoin/issues/32433#issuecomment-2887277091)
Looking at this a bit closer, I see that this shouldn't be an issue unless you also have `-cjdnsreachable` set. It is currently an issue because we process `rpcallowip` before we even check for `cjdsnreachable`... I'm going to write a patch and open a PR shortly.
(https://github.com/bitcoin/bitcoin/issues/32433#issuecomment-2887277091)
Looking at this a bit closer, I see that this shouldn't be an issue unless you also have `-cjdnsreachable` set. It is currently an issue because we process `rpcallowip` before we even check for `cjdsnreachable`... I'm going to write a patch and open a PR shortly.
💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2093440637)
oh I see, forgot about that check - can be resolved.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2093440637)
oh I see, forgot about that check - can be resolved.
👍 stickies-v approved a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2847037118)
ACK, nice cleanup.
Introduces some slight behaviour change when passing integer arguments through startup options, the REST interface, as and through `bitcoin-cli` and `bitcoin-tx`. While some of these changes _may_ break existing user flows that were previously fine, they should hopefully be quite rare, and I think are worth the cleanup and more strict parsing.
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2847037118)
ACK, nice cleanup.
Introduces some slight behaviour change when passing integer arguments through startup options, the REST interface, as and through `bitcoin-cli` and `bitcoin-tx`. While some of these changes _may_ break existing user flows that were previously fine, they should hopefully be quite rare, and I think are worth the cleanup and more strict parsing.
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093391212)
If we use `SAFE_CHARS_URI` here (seems appropriate since we're dealing with a uri) the `+` sign is returned in the error description:
<details>
<summary>git diff on fa24012c18</summary>
```diff
diff --git a/src/rest.cpp b/src/rest.cpp
index b340567bd1..464880224c 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -964,7 +964,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
const auto blockheight{ToIntegral<int32_t>(height_str)};
if (!bl
...
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093391212)
If we use `SAFE_CHARS_URI` here (seems appropriate since we're dealing with a uri) the `+` sign is returned in the error description:
<details>
<summary>git diff on fa24012c18</summary>
```diff
diff --git a/src/rest.cpp b/src/rest.cpp
index b340567bd1..464880224c 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -964,7 +964,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
const auto blockheight{ToIntegral<int32_t>(height_str)};
if (!bl
...
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093431546)
nit: looks safe because we're comparing with `tx.vin.size()`, but using `size_t` seems more appropriate for a container index? (+ for `MutateTxDelOutput`)
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093431546)
nit: looks safe because we're comparing with `tx.vin.size()`, but using `size_t` seems more appropriate for a container index? (+ for `MutateTxDelOutput`)
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093425957)
nit: I think making this a `size_t` simplifies it a bit more?
<details>
<summary>git diff on fa24012c18</summary>
```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 816f11045a..9e2c6d0902 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -229,13 +229,13 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
{
- const auto idx{ToIntegral<
...
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093425957)
nit: I think making this a `size_t` simplifies it a bit more?
<details>
<summary>git diff on fa24012c18</summary>
```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 816f11045a..9e2c6d0902 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -229,13 +229,13 @@ static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
{
- const auto idx{ToIntegral<
...
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093439082)
It seems we now no longer have `ToIntegral` fuzz test coverage. It's a fairly simple function, so maybe that's fine, but so was `ParseIntegral`. Reckon it's worth keeping `ToIntegral` for the most common types in this harness?
(https://github.com/bitcoin/bitcoin/pull/32520#discussion_r2093439082)
It seems we now no longer have `ToIntegral` fuzz test coverage. It's a fairly simple function, so maybe that's fine, but so was `ParseIntegral`. Reckon it's worth keeping `ToIntegral` for the most common types in this harness?
💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2093480971)
Yes
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2093480971)
Yes
💬 achow101 commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2093483166)
With legacy wallets, this tested the `importprivkey` RPC. With descriptor wallets, it tests `importdescriptors` via the wrapper. But `importdescriptors` already has an explicit test on the next line, so this line is unnecessary.
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2093483166)
With legacy wallets, this tested the `importprivkey` RPC. With descriptor wallets, it tests `importdescriptors` via the wrapper. But `importdescriptors` already has an explicit test on the next line, so this line is unnecessary.
💬 pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093487160)
If you do the `self.stop_node` with the warning [in it](https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2845981748) that would do (I've check this is also done in a few .py's - eg `feature_config_args.py`- and the buffer cleanup is done in `mining_mainnet`). Also because then you start the node without the settings, leaving it as before the test, could be cleaner, but up to you, it looks ok this way as well.
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093487160)
If you do the `self.stop_node` with the warning [in it](https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2845981748) that would do (I've check this is also done in a few .py's - eg `feature_config_args.py`- and the buffer cleanup is done in `mining_mainnet`). Also because then you start the node without the settings, leaving it as before the test, could be cleaner, but up to you, it looks ok this way as well.
💬 maflcko commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093494320)
looks like dead code. `master` can never be less than 29
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093494320)
looks like dead code. `master` can never be less than 29
💬 maflcko commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093495170)
why the `.rpc.`?
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093495170)
why the `.rpc.`?
💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2093502405)
Ah, I see. I think that's unnecessary and the current test is sufficient to verify the behavior.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2093502405)
Ah, I see. I think that's unnecessary and the current test is sufficient to verify the behavior.
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093504031)
**sp**an?
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093504031)
**sp**an?
💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2887408696)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2089806374) removing unnecessary descriptor wallet checks.
(https://github.com/bitcoin/bitcoin/pull/32481#issuecomment-2887408696)
Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2089806374) removing unnecessary descriptor wallet checks.
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093515670)
"Aggregate" is used as an imperative verb here, so it is "(you,) aggregate (these) pubkeys"
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093515670)
"Aggregate" is used as an imperative verb here, so it is "(you,) aggregate (these) pubkeys"
📝 furszy opened a pull request: "restrict std::cerr to errors; use std::cout for warnings and info"
(https://github.com/bitcoin/bitcoin/pull/32538)
Warning messages were previously sent to `stderr`, which could cause them to
be misinterpreted as actual errors. This change redirects warning and informational
messages to `stdout` instead, making it explicit that they are not errors and
preventing unintended `stderr` handling.
To verify this behavior, added a test case triggering the simplest init warning I could find
(one that was actually not previously covered by tests): the `-bind` "bad-port" message.
So, running the test commit on
...
(https://github.com/bitcoin/bitcoin/pull/32538)
Warning messages were previously sent to `stderr`, which could cause them to
be misinterpreted as actual errors. This change redirects warning and informational
messages to `stdout` instead, making it explicit that they are not errors and
preventing unintended `stderr` handling.
To verify this behavior, added a test case triggering the simplest init warning I could find
(one that was actually not previously covered by tests): the `-bind` "bad-port" message.
So, running the test commit on
...
💬 furszy commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093528039)
Created #32538 as an alternative approach. Warning messages could be redirected to stdout instead of being sent through stderr. I'm not seeing a reason to treat them as errors.
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2093528039)
Created #32538 as an alternative approach. Warning messages could be redirected to stdout instead of being sent through stderr. I'm not seeing a reason to treat them as errors.