💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r2093366846)
I think it is just used for logging. I could just stub it out, like we do for the translation function, but I am also not as sure how important it really is to be printing the clientversion there for these logs.

💬 fanquake commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r2093371205)
Yea, we could probably drop that. There's also `CLIENT_VERSION` fed into the env RNG, which could also likely be dropped.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2093372479)
Script checks are only skipped when the current block is roughly 2 weeks away from the tip. See https://github.com/bitcoin/bitcoin/blob/3f83c744ac28b700090e15b5dda2260724a56f49/src/validation.cpp#L2476-L2494

We have to bury the test block 2 weeks deep for script verification to the skipped

👍 fanquake approved a pull request: "ci: Enable feature_init and wallet_reorgsrestore in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/32519#pullrequestreview-2847192390)
ACK fa2be605fee42c1286de2ddefbec976dde2c35ba - x86_64, aarch64

🚀 fanquake merged a pull request: "ci: Enable feature_init and wallet_reorgsrestore in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/32519)

👍 fanquake approved a pull request: "scripted-diff: Remove unused leading newline in RPC docs"
(https://github.com/bitcoin/bitcoin/pull/32514#pullrequestreview-2847195929)
ACK fa1f10a49e7c4f6377fbc7ae2f1520b38c86e5fa

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2887255441)
Rebased to see if that fixes CI failure (which seems unrelated), the branch was previously rebased on Mar 17 2025.

💬 fanquake commented on pull request "Update leveldb subtree to latest upstream":
(https://github.com/bitcoin/bitcoin/pull/32534#issuecomment-2887264548)
Guix Build
```bash
a12f2290f4e5267f2ba9ad9da0d95e7990ea3e5772d7658a133d607a25afe7b4 guix-build-7015052eba23/output/aarch64-linux-gnu/SHA256SUMS.part
6232c5017a17753b33c62f32def946a72f70ce63b76b55dc9a8fa8c8e75229fe guix-build-7015052eba23/output/aarch64-linux-gnu/bitcoin-7015052eba23-aarch64-linux-gnu-debug.tar.gz
c702cf46c5c463ccd08ec97f3858cf1a1ce612e0c92836f6dedc94d1f6ad16f3 guix-build-7015052eba23/output/aarch64-linux-gnu/bitcoin-7015052eba23-aarch64-linux-gnu.tar.gz
a0e2e6f5198c435c1
...

🚀 fanquake merged a pull request: "[28.x] 28.2rc1"
(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

💬 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.

💬 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.

👍 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.

💬 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
...

💬 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`)

💬 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<
...

💬 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?

💬 achow101 commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(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.

💬 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.