💬 maflcko commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#issuecomment-2726465393)
lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1996815658)
> This (+ in log_category_to_string() leads to runtime assertion errors for interpreted languages

I think the problem here is that the C enums are weakly typed, and if you use them in weakly typed languages you run into these problems. I think the function signature should already give enough of a hint on what the range of allowed values is. That said, I'll change this to instead return `std::nullopt` ad then return false from there.

💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726501066)
review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK d5537c18a903
...

💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2726515276)
(In theory this could be backported to 29.0 to avoid a fuzz timeout there, but it isn't too important, up to the backport maintainer)

⚠️ bearycool11 opened an issue: "Feature Request: Forking Bitcoin for Native gRPC/dRPC Integration"
(https://github.com/bitcoin/bitcoin/issues/32076)
### Please describe the feature you'd like to see added.

So I noticed on mintscan that we have bitcoin API, but... we don't have gRPC/dRPC hooking in bitcore, which is causing a lot of wacky stuff to occur on Coinbase.

Then again, if we're going to introduce this feature we to be careful of this following bug vulnerability

interface ProviderRpcError {
message: string;
code: number;
data?: { method?: string };
stack?: string;
}

Because we all know what that does. Coinbase, the cryp
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1996842801)
I don't think we need to rely on the order inside the enumeration here. That said, it could also just mirror the values in the `BCLog::Level`. I left out `Warning` and `Error` because you can't really control those right now.

💬 janb84 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2726538829)
> Thanks for working on this!
>
> > ... After running through the steps in this guide, you are encouraged to do your own testing.
> > This can be as simple as testing the same features in this guide but trying it a different way. ...
>
> These seem like they belong in the same paragraph? Either replace "This" with "Your own testing" or bring them together.

Changed ✅
>
> > introduces ephemeral dust to improve transaction packaging
>
> Ephemeral dust transactions should be combined with a tra
...

💬 bearycool11 commented on issue "Feature Request: Forking Bitcoin for Native gRPC/dRPC Integration":
(https://github.com/bitcoin/bitcoin/issues/32076#issuecomment-2726539486)
as for the coinbase SDK with the AI model causing further mayhem because I'm quite sure they didn't even look at my repos and just let the thing loose.... yeah that's another problem and bug we will need to resolvde-- I mean if you all just used Dr. Steve Scargal's and my adaptive persistent memory architecture, we wouldn't be having these issues with bugwatcher.c/.h and custodian.c/h,

Also the Github Staff needs to get off their butts and approve me so I can get sponsored and paid for this st
...

✅ pinheadmz closed an issue: "Feature Request: Forking Bitcoin for Native gRPC/dRPC Integration"
(https://github.com/bitcoin/bitcoin/issues/32076)

💬 pinheadmz commented on issue "Feature Request: Forking Bitcoin for Native gRPC/dRPC Integration":
(https://github.com/bitcoin/bitcoin/issues/32076#issuecomment-2726551121)
gRPC is a reasonable feature request and has been discussed before:
https://github.com/bitcoin/bitcoin/issues/29912
https://github.com/bitcoin/bitcoin/issues/16719

This issue was closed due to the personal information involved. It is reasonable to have a discussion on github about this techincal topic as long as it stays techincal.

📝 sekomer opened a pull request: "style: Fix spacing in script.h for OP_TRUE definition"
(https://github.com/bitcoin/bitcoin/pull/32077)
This pull request fixes a spacing issue in `script.h` where the definition for `OP_TRUE` was formatted without a space before the assignment operator. The only modification is changing:

```diff
- OP_TRUE=OP_1,
+ OP_TRUE = OP_1,
```

This update improves the code's readability and adheres to the project's style guidelines without affecting functionality.

💬 Sjors commented on pull request "test: avoid disk space warning for non-regtest":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2726726152)
Implemented @maflcko's suggestion of using prune to prevent the disk warning rather than deleting it from the log.

I also found the same issue in `feature_signet` when using the suggested 4 GB instead my usual bigger RAM disk.

💬 fjahr commented on pull request "test: avoid disk space warning for non-regtest":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2726736148)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89

Cool, that's a simpler solution.

💬 Sjors commented on issue "bumpfee behavior with "Subtract fee from amount"":
(https://github.com/bitcoin/bitcoin/issues/11122#issuecomment-2726805354)
Partially. The the new `original_change_index` can be used here, except when the original transaction didn't have change.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1997117045)
Just noticed that the same is also true for the logger, but unlike the the options, I can actually see multiple threads accessing it. I don't think we should fix that now though, to me it feels more important to have non-global logging objects first.

💬 eyedeekay commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2726907987)
I agree with zzz, the concept ACK is fine but I'm very concerned about having the session bank be 10 sessions long. I think the best method of managing the session back sort of depends on the rate at which you'll need them.

If you're creating sessions fast enough to need a bank of 10 sessions, then I think at some point you have no choice but to keep the sessions alive longer and re-use them more. On the other hand, if you aren't creating them fast enough to need a bank of 10 sessions, then k
...

⚠️ mrtnetwork opened an issue: "listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet"
(https://github.com/bitcoin/bitcoin/issues/32078)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

When running the command:
```
bitcoin-cli listdescriptors true
```
it fails with the error:
```
error code: -4
error message:
Can't get descriptor string.
```
even though the wallet is not watch-only and is a descriptor wallet.

However, when running:
```
bitcoin-cli listdescriptors false

```
the command works fine and returns descriptor information, but without private keys.



### Expec
...

💬 yancyribbens commented on pull request "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags":
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2726931169)
I'm curious if this would have fixed my issue: https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781 which sparked the discussion https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2703801270. As I remember, I had successfully built, and then I was trying to rebuild for fuzzing using clang-16 instead, which was causing a build error. It was resolved by nuking the build folder, although the discussion in https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-27
...

💬 yancyribbens commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2726938039)
> We've recently discussed auto-generating parts of the header and library code instead of writing it by hand as done here. I think for exposing some of the consensus-related constants in that manner might be a good way forward eventually.

@TheCharlatan thanks for the reply. How would auto-generating parts work? That does sound potentially promising as a way to build rust crates as well if we can use the same input data for auto-generating.

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1997238955)
could also use a const for these magic numbers.

```
P2WPKH_VB_SIZE = 31
```