⚠️ l0rinc opened an issue: "Corecheck isn't run for latest PRs"
(https://github.com/bitcoin/bitcoin/issues/33359)
See: https://corecheck.dev/bitcoin/bitcoin/pulls/33353
I have pushed a few changes that could fix it, reviews are welcome: https://github.com/corecheck/corecheck/pulls
(https://github.com/bitcoin/bitcoin/issues/33359)
See: https://corecheck.dev/bitcoin/bitcoin/pulls/33353
I have pushed a few changes that could fix it, reviews are welcome: https://github.com/corecheck/corecheck/pulls
👍 TheCharlatan approved a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3207912876)
ACK 8cb75ee0a63880b55ac695fe884748aa7d604c38
The log message is a bit clunky though. Since it now appears on startup I think it would be nice if it also mentioned that this just reflects assumevalid.
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3207912876)
ACK 8cb75ee0a63880b55ac695fe884748aa7d604c38
The log message is a bit clunky though. Since it now appears on startup I think it would be nice if it also mentioned that this just reflects assumevalid.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3276630793)
> that this just reflects assumevalid
Unfortunately it's more complicated than that, see https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3273277122
If you have a concrete suggestion for how to rephrase it, I don't mind applying it.
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3276630793)
> that this just reflects assumevalid
Unfortunately it's more complicated than that, see https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3273277122
If you have a concrete suggestion for how to rephrase it, I don't mind applying it.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2337975289)
This seems less racy, indeed, updated, added you as coauthor, thanks
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2337975289)
This seems less racy, indeed, updated, added you as coauthor, thanks
🤔 nervana21 reviewed a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3208228592)
got it, thanks!
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3208228592)
got it, thanks!
📝 jalateras opened a pull request: "rpc: Add validation for invalid taproot signatures in analyzepsbt"
(https://github.com/bitcoin/bitcoin/pull/33360)
## Summary
This PR fixes issue #33320 by adding explicit validation for invalid taproot key path signatures in the `analyzepsbt` RPC command.
## Problem
Currently, when a PSBT contains an invalid `taproot_key_path_sig`, `analyzepsbt` returns a confusing status of `"next": "updater"` instead of properly reporting the validation error. This can happen when external applications incorrectly construct PSBTs.
## Solution
- Added validation in `AnalyzePSBT` to check for invalid taproot key path sign
...
(https://github.com/bitcoin/bitcoin/pull/33360)
## Summary
This PR fixes issue #33320 by adding explicit validation for invalid taproot key path signatures in the `analyzepsbt` RPC command.
## Problem
Currently, when a PSBT contains an invalid `taproot_key_path_sig`, `analyzepsbt` returns a confusing status of `"next": "updater"` instead of properly reporting the validation error. This can happen when external applications incorrectly construct PSBTs.
## Solution
- Added validation in `AnalyzePSBT` to check for invalid taproot key path sign
...
💬 ajtowns commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3277131901)
> My hesitation was mostly about re-using the coins cache as is. In my opinion it is the wrong data structure to use in this case. Potential clients of the kernel are often capable of supplying the coins in order as consumed by a transaction's inputs.
I think that's a false optimisation for two reasons: first, is that the primary client of the bitcoin core kernel is the bitcoin core node software, we should be designing for that case, not for other hypothetical clients; second, looking up a m
...
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3277131901)
> My hesitation was mostly about re-using the coins cache as is. In my opinion it is the wrong data structure to use in this case. Potential clients of the kernel are often capable of supplying the coins in order as consumed by a transaction's inputs.
I think that's a false optimisation for two reasons: first, is that the primary client of the bitcoin core kernel is the bitcoin core node software, we should be designing for that case, not for other hypothetical clients; second, looking up a m
...
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338113)
Added something like this, though without the "88"
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338113)
Added something like this, though without the "88"
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338321)
Added
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338321)
Added
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338802)
Round-trip assertions in both directions added to fuzz/script_flags.cpp
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2338338802)
Round-trip assertions in both directions added to fuzz/script_flags.cpp
💬 l0rinc commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2338351098)
Instead of the current single-entry LevelDB batching (creating a batch, serializing the `DBHeightKey` and `std::pair<uint256, DBVal>` and writing the batch, containing a single entry), I have extracted the `CDBBatch` outside of the loops and committing and writing *that* regularly, see https://github.com/l0rinc/bitcoin/pull/37/files for a draft (I will need to break into commits and maybe adjust some tests if needed).
I will run a follow-up benchmark to see how well this performs, since the o
...
(https://github.com/bitcoin/bitcoin/pull/33306#discussion_r2338351098)
Instead of the current single-entry LevelDB batching (creating a batch, serializing the `DBHeightKey` and `std::pair<uint256, DBVal>` and writing the batch, containing a single entry), I have extracted the `CDBBatch` outside of the loops and committing and writing *that* regularly, see https://github.com/l0rinc/bitcoin/pull/37/files for a draft (I will need to break into commits and maybe adjust some tests if needed).
I will run a follow-up benchmark to see how well this performs, since the o
...
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3277968407)
I have instrumented the code, ran some RPC commands and reindexed the whole thing again to see why script verification is always on.
After some discussions with @ajtowns it turns out I needed both `-assumevalid=<in-range header>` and `-minimumchainwork=0` to be able to disable script verification.
---
After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
The default `assumevalid` block (height 912,683) is in that missing range, so the header lookup fails
...
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3277968407)
I have instrumented the code, ran some RPC commands and reindexed the whole thing again to see why script verification is always on.
After some discussions with @ajtowns it turns out I needed both `-assumevalid=<in-range header>` and `-minimumchainwork=0` to be able to disable script verification.
---
After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
The default `assumevalid` block (height 912,683) is in that missing range, so the header lookup fails
...
🤔 ajtowns reviewed a pull request: "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup"
(https://github.com/bitcoin/bitcoin/pull/33324#pullrequestreview-3208824857)
Having it be a startup option like -reindex seems fine to me.
(https://github.com/bitcoin/bitcoin/pull/33324#pullrequestreview-3208824857)
Having it be a startup option like -reindex seems fine to me.
💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338618798)
Would be good to try to reset the timestamp of new_blocks to match that of old_blocks here.
```c++
// attempt to preserve timestamp
fs::last_write_time(file + suffix, fs::last_write_time(file));
```
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338618798)
Would be good to try to reset the timestamp of new_blocks to match that of old_blocks here.
```c++
// attempt to preserve timestamp
fs::last_write_time(file + suffix, fs::last_write_time(file));
```
💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338622414)
Rather than reading the entire blockfile into memory at once, consider chunking it:
```c++
size_t left = fs::file_size(file);
while (left > 0) {
size_t chunk = std::min<size_t>(left, 2 * MAX_BLOCK_SERIALIZED_SIZE);
buf.resize(chunk);
old_blocks.read(buf);
new_blocks.write_buffer(buf);
left -= chunk;
}
```
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338622414)
Rather than reading the entire blockfile into memory at once, consider chunking it:
```c++
size_t left = fs::file_size(file);
while (left > 0) {
size_t chunk = std::min<size_t>(left, 2 * MAX_BLOCK_SERIALIZED_SIZE);
buf.resize(chunk);
old_blocks.read(buf);
new_blocks.write_buffer(buf);
left -= chunk;
}
```
💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338593448)
I was surprised when it hit 100% of the undo files then restarted on the block files and was much slower -- might be better to do the slow files first, or ideally to do block and undo files intermixed so you just have a single 0% to 100% run.
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338593448)
I was surprised when it hit 100% of the undo files then restarted on the block files and was much slower -- might be better to do the slow files first, or ideally to do block and undo files intermixed so you just have a single 0% to 100% run.
💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338660825)
This doesn't seem like it would work correctly with pruning, when blk0000.dat has been deleted?
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338660825)
This doesn't seem like it would work correctly with pruning, when blk0000.dat has been deleted?
💬 l0rinc commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338680285)
We could do that with the recently introduced buffered readers - but that's considerably slower.
Is it a problem to read all of it in memory when we don't even have `dbcache` yet? The total memory usage is just 160 MB during migration, we should be fine until 1 GB at least, right?
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338680285)
We could do that with the recently introduced buffered readers - but that's considerably slower.
Is it a problem to read all of it in memory when we don't even have `dbcache` yet? The total memory usage is just 160 MB during migration, we should be fine until 1 GB at least, right?
💬 ajtowns commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3278199407)
> After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
> The default `assumevalid` block (height 912,683) is in that missing range,
I think the behaviour here is probably fine -- it's a weak way of preventing you from triggering assumevalid behaviour on an invalid chain with substantially lower total work than the main chain. It might be nice to have a warning that your headers chain doesn't meet minchainwork though. It would probably be nicer to be able to
...
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3278199407)
> After `reindex`, my node has ~841k headers, a lot lower than the current tip of ~914k.
> The default `assumevalid` block (height 912,683) is in that missing range,
I think the behaviour here is probably fine -- it's a weak way of preventing you from triggering assumevalid behaviour on an invalid chain with substantially lower total work than the main chain. It might be nice to have a warning that your headers chain doesn't meet minchainwork though. It would probably be nicer to be able to
...
💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338802041)
I don't think buffered readers is the right thing (that's for when you want to process small amounts of data while still reading it from the file in large chunks), and trying the above didn't seem particularly slow to me.
I guess it could be simplified a bit to:
```c++
buf.resize(2 * MAX_BLOCK_SERIALIZED_SIZE);
while (true) {
size_t size = old_blocks.detail_fread(buf);
if (size == 0) break;
new_blocks.write_buffer(std::span(buf, 0, size));
}
```
(https://github.com/bitcoin/bitcoin/pull/33324#discussion_r2338802041)
I don't think buffered readers is the right thing (that's for when you want to process small amounts of data while still reading it from the file in large chunks), and trying the above didn't seem particularly slow to me.
I guess it could be simplified a bit to:
```c++
buf.resize(2 * MAX_BLOCK_SERIALIZED_SIZE);
while (true) {
size_t size = old_blocks.detail_fread(buf);
if (size == 0) break;
new_blocks.write_buffer(std::span(buf, 0, size));
}
```