💬 ismaelsadeeq commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1431646003)
This is fixed in https://github.com/bitcoin/bitcoin/pull/29115

💬 jamesob commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863092993)
Concept ACK. I better blow the dust off of bitcoinperf...

🤔 ryanofsky reviewed a pull request: "doc: Add multiprocess design doc"
(https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1788797844)
re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1777912181

> Approach ACK. I'm still working on my understanding of multiprocess but this document was very helpful already in its current state, no major suggestions at this point.

Thank you, applied your suggestions, and improved the example flow section based on your feedback.

re: https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1782842554

> ACK [837c53d](https://github.com/bitcoin/bitcoin/commit/83
...

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361667)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424253642

> Steps 2 and 3 highlight what's handled by generated code, for consistency that might be helpful in steps 4&5 too?

Thanks, added more details to this section so it now mentions the generated client and server classes and references the generated code consistently.

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361020)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424248273

> typo nit

Thanks, fixed

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360695)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427363274

> nit

Useful clarification, added

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361157)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428164608

>

Thanks, fixed

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361546)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1424249827

> nit: rogue backtick

Thanks, fixed

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360934)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428162298

> nit: typo

Thanks, fixed

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361251)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428165515
>

Thanks, fixed

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431361433)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1428187716

> Maybe mention that his is based on code that is not merged yet, for example `chain.capnp` is linked below but it doesn't exist on master yet.

Oops, this is not good. I changed the link to point to the branch version for now since I think seeing the interface definition can be helpful.

💬 ryanofsky commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1431360480)
re: https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1427365136

> nit suggestions:
>
>* 'central to the bitcoin network' isn't really relevant to this doc
>
>* I hear people complain that big changes are always a security risk in the short term all the time

Yes, definitely agree and took the suggestion.

💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1863110548)
Benchmarks are also done by https://corecheck.dev/bitcoin/bitcoin/pulls/29114 :)

Though, to test this, one would have to manually apply the diff either way, so local running is probably the easiest.

(I don't think `std::vector<std::byte>` serialization is used anywhere, where performance matters or is measured)

👍 ismaelsadeeq approved a pull request: "Add multiplication operator to CFeeRate"
(https://github.com/bitcoin/bitcoin/pull/29037#pullrequestreview-1789316545)
ACK 1757452cc55a6dacc62e4258043ee4d711fd281a

💬 ismaelsadeeq commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1431686247)
nit: will be nice to squashed this to the first commit

💬 achow101 commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1863129994)
Do you see any `hdkeypath` that doesn't follow the pattern `m/a'/b'/c'`?

💬 ryanofsky commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1863153024)
> cc @ryanofsky

I'd like to help with this, but I just don't understand most of the discussion so far. I see that this fixes some kind of a bug on windows and is supposed to have other benefits mentioned in the PR description, but I don't see a clear description of the bug anywhere. Maybe someone can open an issue with a description of the bug and why it happens? The issue could links to whatever PRs have been proposed to fix it.

💬 brandonblack commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1863182118)
ACK
(Re-upping)

💬 ryanofsky commented on issue "Unnecessary symbols being exported in `libbitcoinconsensus.so`":
(https://github.com/bitcoin/bitcoin/issues/26698#issuecomment-1863192161)
From the linked issue https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36022#c4:

> For C++, types that are to be used across shared libraries have to be visible

Seems to suggest that it is a good thing for these symbols to be exported so the library and its callers use the same symbols, particularly `type_info` symbols, and I guess generally so symbols in shared libraries have the same visibility as symbols in static libraries. So linking dynamically doesn't turn shared symbols into hidden sym
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1863197392)
@Fi3 found the issue, fixed in a7f08e74428c4e5da62365d686e5c51c5afc959d

```patch
--- a/src/common/sv2_noise.cpp
+++ b/src/common/sv2_noise.cpp
@@ -56,11 +56,11 @@ void Sv2CipherState::EncryptMessage(Span<std::byte> input, Span<std::byte> outpu
for (size_t i = 0; i < num_chunks; ++i) {
size_t chunk_start = i * max_chunk_size;
size_t chunk_end = std::min(chunk_start + max_chunk_size, input.size());
size_t chunk_size = chunk_end - chunk_start;
con
...