💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1848782387)
Done

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1848782476)
Done

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1848782538)
Done

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1848782692)
Done

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1848782930)
Renamed

💬 pinheadmz commented on issue "Feature request: Backup of datadir state":
(https://github.com/bitcoin/bitcoin/issues/31324#issuecomment-2486339380)
I think backups should be the users job not bitcoind. Especially putting backups in datadir -- wouldn't it make more sense to be on a completely different drive ?

💬 Sjors commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2486349343)
Noticed while setting up an new guix environment for #31323 that I needed a few more (starting at 1.4.0 installed on Ubuntu in a VM):

```
guix download https://www.gnupg.org/ftp/gcrypt/gnutls/v3.7/gnutls-3.7.2.tar.xz
guix download https://gnupg.org/ftp/gcrypt/libgcrypt/libgcrypt-1.8.8.tar.bz2
guix download https://gnupg.org/ftp/gcrypt/libgpg-error/libgpg-error-1.42.tar.bz2
```

(will add more here if needed, still building)

💬 Sjors commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2486363936)
On my x86_64 machine I get the same guix hashes as @fanquake.

💬 Sjors commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2486378063)
This could be a problem... I can't find `net-tools-1.60-0.479bb4a.zip.drv` anywhere on the internet, so I had to copy it from my other guix machine instead.

So the problem may not be limited to backport releases like v27.2

💬 ryanofsky commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848821236)
Are you are seeing a difference in practice? At least according to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for, `wait_for(lock, rel_time, pred)` is equivalent to `wait_until(lock, rel_time + now, pred)` and according to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_until that should be equivalent to `while(!pred()) wait...` checking the predicate before there is any waiting.

💬 ryanofsky commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486457573)
re: https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2485712161

> Otherwise we'd never get past this point:

I think we typically get past that point on line 1795 in `AppInitMain` even though `m_tip_block` is still null at that point, because when the node restarted the `chainman.ActiveTip() == nullptr` check will normally be false. This is because of the `InitAndLoadChainstate` call on line 1624, which calls `Chainstate::LoadChainTip()` and sets the active tip to `coins_cache.Get
...

👍 ryanofsky approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2446300488)
Code review ACK 9aa50152c1cfa1c41215b2b51ed7a516aa67137a

💬 achow101 commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2486474269)
ACK 111465d72dd35e42361fc2a089036f652417ed37

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848870979)
I thought I did, but give what the documentation says, it indeed shouldn't be needed. Will test again.

📝 Sjors converted_to_draft a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)
It's useful to be able to wait for an active chaintip to appear, by calling ` waitTipChanged(uint256::ZERO);`.

Unfortunately this doesn't work out of the box, because `notifications().m_tip_block` is `ZERO` until a new block arrives.

Additionally we need an early return before calling `wait_for`.

👍 ryanofsky approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2446313068)
Concept ACK . It seems like a nice improvement to not require specifying an option that won't typically be used. But I'm a little hazy on the details of this. I think better documentation could help. Left a suggestion and some questions below.

💬 ryanofsky commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1848878221)
re: https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846813317

Would it make sense to declare the field as `CScript coinbase_output_script{CScript{} << OP_TRUE}` instead of `std::optional<CScript> coinbase_output_script{};` just to be clearer about what the default value is? Or maybe just add documentation to say what happens if this is not set?

I think the drawback of using std::optional here is that it's not really clear what happens if the option is not set. It's also not clea
...

🚀 achow101 merged a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893)

📝 Sjors opened a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325)
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486525961)
It's a bit tricky at the moment to test the interaction of multiple pull requests on my stratum v2 branches. I'll test this again later, and most likely close the PR then.

Meanwhile I extracted the make `m_tip_block std::optional` commit into #31325 for separate consideration.