💬 hodlinator commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372386020)
https://github.com/bitcoin/bitcoin/actions/runs/11014291937/job/30584604403?pr=30956#step:10:930
contains:
`bitcoind.vcxproj -> D:\a\bitcoin\bitcoin\build\src\RelWithDebInfo\bitcoind.exe`
...notice the "RelWithDebInfo", so it should be generating .PDB files like it does for me locally.

@maflcko continuing discussion after [your comment in the related issue](https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326700782):
> Seems fine to integrate in a pull request and then keep t
...

💬 instagibbs commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2372438645)
here's a link to the first(?) discussion of this rule 2 evasion technique at least I'm aware of: https://github.com/bitcoin/bitcoin/pull/23121#issuecomment-929475999

💬 davidgumberg commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372441698)
I think you can add something like:

```yml
- uses: actions/upload-artifact@v4
with:
name: dump
path: %RUNNER_TEMP%/**/bitcoind.dmp
if-no-files-found: ignore
```
after the `run:` part of each job.

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/storing-and-sharing-data-from-a-workflow
https://github.com/actions/upload-artifact#examples

🤔 theStack reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2326570451)
I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.

Might be worth to update the PR description with benchmark results from release builds,
...

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1774189607)
naming nit: any reason why sometimes the `WithDB` postfix is added for the new methods taking a `WalletBatch`, while on other cases in later commits (e.g. `DeleteRecords`) it's overloaded with the already-existing name? Personally I'm not a huge fan of overloading, but happy to ACK either variant.

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1774150203)
preprocessor nit, as one-liner:
```suggestion
#if defined(USE_BDB) && defined(USE_SQLITE)
```

💬 theStack commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1774150882)
nit
```suggestion
CKey key = GenerateRandomKey();
```

💬 tdb3 commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#discussion_r1774225698)
Thanks for taking a look. Decided to remove "(other options)" since it should be pretty clear to readers that the presence of this option wouldn't preclude the use of other ones needed.

💬 kevkevinpal commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2372825515)
> What is the state of this?

I have been busy for the last couple of weeks I can take a look at it and finish up to make it ready for review in the next week or so, sorry for the delay

💬 Eunovo commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2372973840)
I retested this PR @https://github.com/bitcoin/bitcoin/pull/30688/commits/de85ebeff481c4afa1b6b3ea9470333692e4f099 against master using the qa-assets https://github.com/bitcoin-core/qa-assets from and https://github.com/brunoerg/qa-assets/pull/1

This PR vs Master on https://github.com/bitcoin-core/qa-assets
```
#2419155 REDUCE cov: 3972 ft: 22225 corp: 1718/24Mb lim: 964552 exec/s: 90 rss: 1015Mb L: 94/949713 MS: 4 CopyPart-ChangeASCIIInt-ChangeBinInt-EraseBytes-
```
Master
```

...

💬 maflcko commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2373131525)
> 1. Do you think the current approach of temporarily changing from Release -> RelWithDebInfo for Windows would be acceptable, as a PR that is only run on CI?

Seems fine to do permanent, if there are no downsides or slow-down. cc @hebasto

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2373259581)
I'll work on a followup for the above things.

💬 maflcko commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2373371985)
> I'll work on a followup for the above things.

Already started working on this, because I needed it for other stuff.

Happy to drop mine, if you are done already.

📝 Sjors opened a pull request: "Followups for #30409 waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/30966)
Based on comments on #30409.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2373386581)
@maflcko #30966

💬 Sjors commented on pull request "Followups for #30409 waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/30966#issuecomment-2373388800)
@theuni also wrote:

> Related: It's a shame that the condvar/mutex leak out of here. Is there a reason not to add getter/waiter/notifier functions?

Not sure how to go about that, but happy to take a patch.

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2373443353)
re-utACK fbed31494af9acf6bd543801143a12399b2c6a1a

I didn't check if the new `CustomBuildField` and `CustomReadField` to serialize `std::chrono` is identical to that in #30437. If not, then I'll test it next time I rebase https://github.com/Sjors/bitcoin/pull/48.

📝 maflcko opened a pull request: " refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967)
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.

So remove it, along with some other dead code, as well as minor fixups.

👍 dergoegge approved a pull request: "doc: Adjust links in OSS-Fuzz section"
(https://github.com/bitcoin/bitcoin/pull/30963#pullrequestreview-2327621768)
ACK fa6c1946d235c6ea5b9384d8488d80aa3bcd0ad7

🤔 maflcko reviewed a pull request: "Followups for #30409 waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/30966#pullrequestreview-2327623918)
I've opened https://github.com/bitcoin/bitcoin/pull/30967, which includes the follow-ups here, but I solved them in another way.

Feel free to take or leave them. I can drop them from mine, once this one is merged.