💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600513951)
Expanded the comment with the `fKnown` behavior.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600514427)
done as suggested.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600514845)
moved to the doc commit.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#issuecomment-2110943056)
[9cf475f ](https://github.com/bitcoin/bitcoin/commit/9cf475ffffb869cd55c2b2f3be84d7c90b199521)to [e41667b](https://github.com/bitcoin/bitcoin/commit/e41667b720372dae8438ea86e9819027e62b54e0):
Addressed review feedback.

💬 pinheadmz commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2110960190)
force-push to fix merge conflict which also looked like a git mistake on my part in the first place 😬

👍 hebasto approved a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2056250408)
ACK c5f9afd946efb4eb6b27b2d0316f2f787a9608c7, tested on FreeBSD 14.0.

`./src/bitcoind -logthreadnames` works as expected.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1600564410)
I'm guessing "a peer for some reason does not request reconciliations from us for a long while", hence why it references (1)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1600586129)
This seems to be consistent with how a deterministic randomizer is seeded in many other places in the codebase. What is your rationale for making it different here?

⚠️ edilmedeiros opened an issue: "contrib/signet/miner: grind will fail for high difficulty chain"
(https://github.com/bitcoin/bitcoin/issues/30102)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Mining will fail with a `Could not satisfy difficulty target`.

```bash
❯ ~/2-development/bitcoin/bitcoin-core/contrib/signet/miner --cli "/Users/jose.edil/2-development/bitcoin/bitcoin-core/src/bitcoin-cli -datadir=/Users/jose.edil/2-development/bitcoin/signet-mining-experiments/signet-fix-hashing" generate --address tb1qylfujt900rjxzfxjj7sktpu7dpm2n9j60ch7jt --grind-cmd "/Users/jose.e
...

💬 TheCharlatan commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1600617406)
Are you going to be using this method for other purposes besides tests and as an internal getter?

👍 TheCharlatan approved a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-2056418308)
Nice, reviewing this was fun.
ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa

💬 TheCharlatan commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1600619857)
Just a comment: This does indeed not trigger clang-tidy's recursion check (I'm guessing because of the constexpr condition).

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600629260)
Would it still make sense to call this method and discard the return ed value, like before, when the output param was mutated?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600629456)
Ah, no, I just like this change

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#issuecomment-2111128757)
ACK e41667b720372dae8438ea86e9819027e62b54e0
Great job, love the results of these untangling tasks!

💬 theuni commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2111151797)
@willcl-ark I added a quick/dirty bench to my branch that shows the difference between copies and moves. It seems correct to me.

Do you have any interest in doing the same for memory usage, just as a sanity check that nothing funky is happening low-level? Beyond that, I'm not really sure where to start with your graph... it really doesn't make any sense to me :)

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600675693)
I don't think so, with the way the function is now only called if we don't know the BlockPos already it seems logical for callers to do something with the result. Of course, if someone comes up with such a use case in the future, they can just remove the `[[nodiscard]]` - after all its point is just to prevent future programming errors.

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-2056500485)
Thanks for the review!

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1600674320)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1600617406

> Are you going to be using this method for other purposes besides tests and as an internal getter?

I could see some other uses. Initially this was just added for testing, but then marco pointed out it could be used as an internal getter in https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1497229478 to avoid recursion in read, write, size, etc methods. The other case it might be useful is if you have have a c
...

💬 theuni commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2111184589)
> Concept ACK - Think I prefer fixing this inline, than in global flags / build. I guess we didn't end up making an issue upstream (https://github.com/llvm/llvm-project/issues)? If there is something to link to, would be good to add it here.

Upstream issue filed here: https://github.com/llvm/llvm-project/issues/92182