💬 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();
```
(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.
(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
(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
```
...
(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
(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.
(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.
(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.
(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
(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.
(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.
(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.
(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
(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.
(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.
💬 maflcko commented on pull request "Followups for #30409 waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/30966#discussion_r1774844673)
Seems overly complicated to turn a reference into a pointer, then turn it into std::any, then recover it from any, then assert the pointer isn't null.
It would be better to remove all of this and just pass the reference.
So it would be good to drop this commit or replace it with fadd531a57710809df0e4b027ce30b5524e5f40f, which does the above.
(https://github.com/bitcoin/bitcoin/pull/30966#discussion_r1774844673)
Seems overly complicated to turn a reference into a pointer, then turn it into std::any, then recover it from any, then assert the pointer isn't null.
It would be better to remove all of this and just pass the reference.
So it would be good to drop this commit or replace it with fadd531a57710809df0e4b027ce30b5524e5f40f, which does the above.
💬 maflcko commented on pull request "Followups for #30409 waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/30966#discussion_r1774841423)
Seems verbose to lock this every time. Also, the comment above is still wrong. It would be good to drop this commit, or replace it with fa7dd1d072dc36997e2d5d702e2da529a093c90f, which also fixes up the outdated comment.
(https://github.com/bitcoin/bitcoin/pull/30966#discussion_r1774841423)
Seems verbose to lock this every time. Also, the comment above is still wrong. It would be good to drop this commit, or replace it with fa7dd1d072dc36997e2d5d702e2da529a093c90f, which also fixes up the outdated comment.
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2373566011)
rebased after conflict with https://github.com/bitcoin/bitcoin/pull/30409
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2373566011)
rebased after conflict with https://github.com/bitcoin/bitcoin/pull/30409
💬 TheCharlatan commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2373601290)
Nice, Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2373601290)
Nice, Concept ACK
💬 maflcko commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2373608939)
> The UX for downloading two verification files sucks IMO. I think we should just go for the clearsign option, even if it's exclusive.
See also: https://www.gnupg.org/documentation/manuals/gnupg/Operational-GPG-Commands.html :
> Note: When verifying a cleartext signature, gpg verifies only what makes up the cleartext signed data and not any extra data outside of the cleartext signature or the header lines directly following the dash marker line. The option --output may be used to write out
...
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2373608939)
> The UX for downloading two verification files sucks IMO. I think we should just go for the clearsign option, even if it's exclusive.
See also: https://www.gnupg.org/documentation/manuals/gnupg/Operational-GPG-Commands.html :
> Note: When verifying a cleartext signature, gpg verifies only what makes up the cleartext signed data and not any extra data outside of the cleartext signature or the header lines directly following the dash marker line. The option --output may be used to write out
...
🚀 fanquake merged a pull request: "doc: Adjust links in OSS-Fuzz section"
(https://github.com/bitcoin/bitcoin/pull/30963)
(https://github.com/bitcoin/bitcoin/pull/30963)
💬 fjahr commented on pull request "doc: correct the zmq automatic build info":
(https://github.com/bitcoin/bitcoin/pull/30946#issuecomment-2373628404)
ACK e52ccc0203aefdebccd5a2f7146e58a816539467
(https://github.com/bitcoin/bitcoin/pull/30946#issuecomment-2373628404)
ACK e52ccc0203aefdebccd5a2f7146e58a816539467