💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1809035958)
> In addition, it is important to note that if developers do not help node runners defend themselves against this attack, they may have to resort to misappropriate means to strengthen their defense.

This pull-req is not a defense. In fact, it does the opposite: with compact blocks, rejecting transactions that are very likely to get mined actually increases bandwidth consumption, in particular, peak bandwidth. You can either incur that cost when the transaction is broadcast. Or later, when the
...

💬 petertodd commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809045700)
Note that BIP-301 suffers from an [equivocation attack](https://petertodd.org/2023/drivechains#equivocation-attack). While there are [many other reasons](https://petertodd.org/2023/drivechains) to NACK this pull-req, at minimum the equivocation attack is a sufficient reason alone.

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1391727313)
Sounds good. Corrected.

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1391727015)
Done as suggested. Thanks.

⚠️ jb55 opened an issue: "macOS qt QTimer::stop crash on v26.0rc2"
(https://github.com/bitcoin/bitcoin/issues/28867)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

It crashed randomly

### Expected behaviour

Not to crash randomly

### Steps to reproduce

No idea, I was just syncing the chain.

### Relevant log output

https://cdn.jb55.com/s/adcf1a349cdfc36d.txt

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

v26.0rc2

### Operating system and version

macOS Ventura 13.6

### Machine specif
...

📝 achow101 opened a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868)
A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.

I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it
...

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1391748844)
I'll update it with an alternative, as I don't have an instance with macOS, please let me know if that would work better now.

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1391749053)
Yes! I'll do, but the `reconsiderblock` won't do anything, the issue I see now is that `PIVOT_BLOCKHASH` wasn't initialised so perhaps it fails there.

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1391749127)
Same as in the previous PR #27845. We could add more context in the output message and perhaps we could update the documentation as well:
https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/doc/design/assumeutxo.md?plain=1#L42-L45

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1809202640)
> By the way, at some point when a bash script gets very complicated, it may be better to convert it to Python.

I do agree, but so far I think it's manageable. I'm happy to convert it if more reviewers see other issues there.

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1391753302)
done.

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1391753979)
Done, pls let me know.

💬 pablomartin4btc commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1809212719)
Addressed @Sjors's feedback.

cc @jamesob as he reviewed #27845, @ryanofsky & @theStack

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1809264269)
Rebased over #28207

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795346)
It isn't. When `FundTransaction` extracts the inputs and outputs from `CreateTransction`'s result and puts them into the tx it got, so the version ends up being preserved. But `CreateTransaction` isn't aware of the version selected by the user.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795660)
With miniscript, it does matter since scripts can contain locktimes.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795950)
With miniscript, it does matter, as it controls whether something with `OP_CSV` is spendable.

💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1809279093)
> `0x` is much more standard and recognisable than `[]`.

It's pretty misleading though, since `CScriptNum` is little-endian: if you see `0x0100` you would expect that to be 256, but for us it would actually be non-minimally-encoded 1; For us, `0x0201` would actually be less than `0x0102`, etc.

Note this applies to block hashes, as well, so if you provide block 816651's header and execute HASH256, you end up with `[7d6368795d9675a198bd1dbe3d87dbdbbc3e0f3560a202000000000000000000]`; if you w
...

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391800163)
I'd prefer to leave it as is since it follows the pattern established by the following commits where various things are `Set`.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391802277)
It is actually expected that both can be provided, and not match. We may be funding a transaction with other inputs that already have some signatures (e.g. a multisig where one of the parties is adding another input for fees, and one other party has already signed with sighash_single), so `m_script_sig` or `m_script_witness` may already exist, but are incomplete. `m_weight` may then also be provided, and it wouldn't match since it would account for the signatures that have not yet been added.