💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634458835)
Hi, I am a bit confused as well but I think I have an idea :) I think the approach chosen in this test doesn't work for what it's actually trying to do. Block 299 is marked invalid in the node under test so it's not surprising that the node is stuck on 298. I don't understand how the "divergent" chain could get "rewound" when the alternative "real" chain is still marked invalid. I don't think the dump should mark and invalid block valid but that is what it would need to do in order to sync the c
...

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634462141)
I think this edit here (and in the log below) isn't needed. "all indexes + reindex" is describing the configuration of the node. Divergent chain is just one of the test cases that this node runs through. I think just having the logs in the test you already have is enough.

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634478108)
I would prefer usage of `assert_equal` with the precise height you expect here.

💬 fjahr commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2160189718)
> 29996 doesn't seem optional, but possibly a blocker, see [#29996 (comment)](https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319)

Thanks for notifying about the conversation there. So far it's still only a test PR and not a fix but if that changes I will move it. I think the actual issue discovered there is fixed in https://github.com/bitcoin/bitcoin/pull/30267 and I will put that in blockers for now.

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634517149)
This may be resolved through addressing the comments on the previous test case anyway but for this one also, both chains should be valid.

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634545007)
Thanks @fjahr and @mzumsande, for your help.

> Block 299 is marked invalid in the node under test so it's not surprising that the node is stuck on 298
> [...] So I think the approach here would need to be altered so that the right chain is not marked invalid

Yes, that makes sense. Initially, I didn't want to create a new node for this test and instead wanted to reuse the existing `node2`. To achieve this, I needed to rollback the chain to create a divergent chain from `START_HEIGHT` up t
...

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634549960)
Huh, I didn't see @mzumsande 's post when I wrote mine... Will look into this approach as well.

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634588775)
> Yes, this issue is fixed with your proposed change and my new approach with a new node3. Here is the updated approach:

Sounds good to me in principle but before doing 3 ensure that the node's tip is actually the tip of the divergent chain. This may happen through the node not knowing the real chain, i.e. not being in sync, but I don't think this is what we are actually trying to test here.

Maybe I am blind or interpret the original Todo differently than you but I don't see a simpler appr
...

👍 theStack approved a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2110004373)
Concept and code-review ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf

💬 theStack commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1634596430)
typo nit:
```suggestion
* @param[in] protocol The particular protocol to be used with the socket, third argument to the socket(2) syscall.
```

💬 fjahr commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1634607244)
To clarify the meaning of the Todo:

> Interesting starting states could be loading a snapshot when the current chain tip is:
> - TODO: Not an ancestor of the snapshot block but has less work

Particularly "but has less work" could mean A) less work than the tip of the chain that includes the snapshot or B) less work than the snapshot block itself. I believe A is the more interesting and intended scenario because B only seems to be a state a node can be in if the snapshot block is invalid o
...

🚀 glozow merged a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254)

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#issuecomment-2160485375)
re-ACK fae3a1f0065064d80ab4c0375a9eaeb666c5dd55, I'm ~0 on the latest force push as `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);` so I'd be in favour of deduplicating/cleaning up this logging logic but can be done in follow-up.

💬 Sjors commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2160498553)
tACK 7cbfd7a7ce45ac68d6041f42f468862f5c193d8c

I made a Guix build, as well as a regular cross-compile on Ubuntu 24.04 for Intel macOS, and lightly tested them.

Guix hashes for darwin:

```
048fa3f381098477b8bea25701547f37a6680b33afb9c61ba88cb4a895aae81d guix-build-7cbfd7a7ce45/output/arm64-apple-darwin/SHA256SUMS.part
d540c4c73b98bef810c85b0b10a6e1a004655fb0c8a4709b8f655c6924afef8b guix-build-7cbfd7a7ce45/output/arm64-apple-darwin/bitcoin-7cbfd7a7ce45-arm64-apple-darwin-unsigned.tar.
...

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2160555599)
> Seems okay to me. Presumably should update doc/descriptors.md though, and might also be nice to have some functional tests demonstrating it works end-to-end? (Add some examples to `DESCS` in wallet_miniscript.py maybe?

@ajtowns Will do!

👍 stickies-v approved a pull request: "[27.1] Finalize"
(https://github.com/bitcoin/bitcoin/pull/30222#pullrequestreview-2110202794)
ACK d756a384d2bebe85f2ce0d192e4d31bbbbe750a1

I checked that:

- backports are clean and look correct
- manpages are identical to mine
- release notes are correct

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1634722562)
> Having an `optional` when in one branch you ignore the value and in the other you assume it always has a value doesn't make a lot of sense to me.
>
> Would it make sense to drop `leaf_version` from `TRNodeInfo` and store the leaf version as the first byte of the script? Alternatively, could just drop the optional and default it to 0.

@ajtowns I prefer to drop the optional and default leaf_version to `0`

🚀 glozow merged a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162)

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2160590090)
Rebased

💬 Prabhat1308 commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1634738029)
Since this function is to be called once per peer , we should add a check if there exists a state for this peer .