💬 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.
(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
...
(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
...
(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)
(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.
(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.
(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.
(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.
(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.
(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.
(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
...
(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
...
(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
...
(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.
(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
```
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1997238955)
could also use a const for these magic numbers.
```
P2WPKH_VB_SIZE = 31
```
✅ maflcko closed a pull request: "style: Fix spacing in script.h for OP_TRUE definition"
(https://github.com/bitcoin/bitcoin/pull/32077)
(https://github.com/bitcoin/bitcoin/pull/32077)
💬 luke-jr commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2727002702)
>This should really be looked into, seeing this a lot more with our hardware now that blocks are full
If the chainstate is getting corrupt, that points to a hardware problem, and that is where it should be fixed.
> re-indexing entire blockchain for a corrupt chainstate a few hundred blocks from tip is horrible UX.
The issue here is that it *isn't* reindexing.
>Should only require reindex from last good block db.
Bitcoin Core does not store old block dbs. It has a single chainstate as of the
...
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2727002702)
>This should really be looked into, seeing this a lot more with our hardware now that blocks are full
If the chainstate is getting corrupt, that points to a hardware problem, and that is where it should be fixed.
> re-indexing entire blockchain for a corrupt chainstate a few hundred blocks from tip is horrible UX.
The issue here is that it *isn't* reindexing.
>Should only require reindex from last good block db.
Bitcoin Core does not store old block dbs. It has a single chainstate as of the
...
👍 hodlinator approved a pull request: "test: Update coverage.cpp to drop linux restriction"
(https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2686973172)
ACK ecdbe72916c1cabbf810e892d25fe6eed30b1306
Tested on Linux by running the [recommended](https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136) Rust deterministic unit test coverage utility both without and with coverage support, with expected results.
(https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2686973172)
ACK ecdbe72916c1cabbf810e892d25fe6eed30b1306
Tested on Linux by running the [recommended](https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136) Rust deterministic unit test coverage utility both without and with coverage support, with expected results.
💬 hodlinator commented on pull request "test: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1996374525)
nit: The PR title / description conforms fairly well to the contributing guidelines, but it would be nice if you also came a bit closer with the commit message.
https://github.com/bitcoin/bitcoin/blob/ecdbe72916c1cabbf810e892d25fe6eed30b1306/CONTRIBUTING.md?plain=1#L119-L124 \+ conventions on non-merge commits make me suggest something closer to:
```diff
- Extended ResetCoverageCounters in coverage.cpp with fallback implementation for non-linux linker and removed linux macro limitation.
+
...
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1996374525)
nit: The PR title / description conforms fairly well to the contributing guidelines, but it would be nice if you also came a bit closer with the commit message.
https://github.com/bitcoin/bitcoin/blob/ecdbe72916c1cabbf810e892d25fe6eed30b1306/CONTRIBUTING.md?plain=1#L119-L124 \+ conventions on non-merge commits make me suggest something closer to:
```diff
- Extended ResetCoverageCounters in coverage.cpp with fallback implementation for non-linux linker and removed linux macro limitation.
+
...
💬 hodlinator commented on pull request "test: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1997328341)
Thanks for the thorough explanation, it was helpful!
nit:
This seems to build fine on Linux with/without coverage:
```C++
extern "C" void __llvm_profile_reset_counters(void);
extern "C" void __gcov_reset(void);
```
My guess is that what is important now is that the fallback is `weak`, not the prototype, could that be? Mac CI seems to accept that, but I'm not sure if our CI tests both non-coverage and coverage builds on Mac.
---
Iff the above is a problem on Mac/other platforms thoug
...
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1997328341)
Thanks for the thorough explanation, it was helpful!
nit:
This seems to build fine on Linux with/without coverage:
```C++
extern "C" void __llvm_profile_reset_counters(void);
extern "C" void __gcov_reset(void);
```
My guess is that what is important now is that the fallback is `weak`, not the prototype, could that be? Mac CI seems to accept that, but I'm not sure if our CI tests both non-coverage and coverage builds on Mac.
---
Iff the above is a problem on Mac/other platforms thoug
...