💬 Self-Hosting-Group commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2159928038)
@laanwj Thank you for the explanation.

💬 hebasto commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2159966884)
Concept ACK.

👍 hebasto approved a pull request: "doc: add release note for 29091 and 29165"
(https://github.com/bitcoin/bitcoin/pull/30261#pullrequestreview-2109554265)
ACK 3d4ca62d883b17d983d6f62cdacbd67a483f7d64.

💬 hebasto commented on pull request "doc: add release note for 29091 and 29165":
(https://github.com/bitcoin/bitcoin/pull/30261#discussion_r1634329701)
nit: In the phrase "Clang 15+ or later" the plus sign seems redundant.

💬 Sjors commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2159998831)
> macOS machines was limited to 256GiB

You're confusing the size of the blockchain with the size of the application download. Once you've installed Bitcoin Core, you can throw away the downloaded zip file. So if you just use Bitcoin-QT nothing changes.

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2160080256)
@ryanofsky do you have any thoughts on this (for this PR or a followup)?

> Additionally the template provider needs to wait, to see if there's a new tip, with g_best_block_cv.wait_until. How would I go about moving that into this interface?

📝 fjahr opened a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267)
This was discovered in a discussion in #29996

If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.

While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.

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