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

💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(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

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

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

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

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

💬 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;
}
```

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

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

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

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

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

🤔 w0xlt reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209275659)
Approach ACK

Using `std::optional<bool>` introduces a third state (unset), which guarantees exactly one initial log message—regardless of whether the first real value is true or false.

For reviewers, here’s a simple way to verify the behavior change:

```
mkdir temp_data_dir
./build/bin/bitcoind -regtest -datadir=temp_data_dir -daemon
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir createwallet w
./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir getnewaddress
./build/b
...

📝 jalateras opened a pull request: "Fix #25980: Validate transactions in combinerawtransaction"
(https://github.com/bitcoin/bitcoin/pull/33361)
## Summary

This PR fixes issue #25980 where `combinerawtransaction` would silently accept unrelated transactions and only use the first one, ignoring the rest.

## Problem

When passed two or more unrelated transactions (transactions with different inputs/outputs), `combinerawtransaction` would:
- Take the first transaction as the base
- Silently ignore the unrelated parts of subsequent transactions
- Only merge signatures for matching inputs

This behavior was confusing and could lead to unexp
...

💬 kannapoix commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2339166383)
It looks like the bitcoin binary might not have been built.
As far as I know, BUILD_BITCOIN_BIN=ON is the default, but maybe you built with a non-default setting.
If you build with bitcoin binary explicitly enabled, it should be generated at the path:

```
cmake -B build -DBUILD_BITCOIN_BIN=ON
```

💬 kannapoix commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3278940886)
reACK 5424b4f1ce74c82b7ae01034bbc1592088048128

Tested and reviewed the new functional test.

📝 vasild opened a pull request: "Run feature_bind_port_(discover|externalip).py in CI"
(https://github.com/bitcoin/bitcoin/pull/33362)
This PR includes https://github.com/bitcoin/bitcoin/pull/32757 (first two commits here).

Then, on top of that:

`feature_bind_port_discover.py` and `feature_bind_port_externalip.py` require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using `ifconfig`) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and got
...

💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3279026280)
> I would really like to know if its possible to run this test in CI ...

> `IFF_LOOPBACK` addresses are being filtered out [here](https://github.com/bitcoin/bitcoin/blob/e17fb86382eafb7ccfddb56bca981f6a12201c85/src/common/netif.cpp#L340). So I don't think the problem from https://github.com/bitcoin/bitcoin/issues/31336 is fixed by this as the OP says.

Both resolved in https://github.com/bitcoin/bitcoin/pull/33362

🤔 Eunovo reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3209656734)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114:

Previously, users could only verify that script checks were enabled by confirming the absence of a "Disabling signature validations at block #1" message in the logs, which is a confusing negative confirmation pattern.
This PR adds explicit positive feedback by ensuring an "Enabling signature validations at block #1" message appears when script checks are enabled, making the system state cleare
...