💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2272260642)
The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al. So I opened the suggested approach of removing the block height in #30598 and I pinged other reviewers of the original metadata PR to weigh in shortly.

Ultimately what matters most to me is that we finally get this feature into the hands of users and I am fine to go along with either approach if it avoids further blocking of
...

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706154943)
In d60c6bba04f23f:

tiny nit: could cache the largest providers length instead of looping through the elements again here:

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
--- a/src/script/descriptor.cpp (revision 8208454eae7484d34a162ffa7701157e56e3cb80)
+++ b/src/script/descriptor.cpp (date 1722981112377)
@@ -1818,6 +1818,7 @@
return {};
}
size_t script_size = 0;
+ size_t max_providers_length = 1; // if multipath was
...

🤔 furszy reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2222305361)
still reviewing, only nits so far.

> Although i agree it's more readable now it's unfortunate to ask to rewrite part of the parsing logic only for style purpose after it had 3 ACKs and underwent significant fuzzing.

I think it depends on the proposed changes. If readability is improved substantially, it helps maintenance and might also uncover logical issues that the fuzzer might not easily encounter. I don't think we should settle anything in stone because of a good fuzzing coverage. We
...

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706155751)
In https://github.com/bitcoin/bitcoin/commit/d60c6bba04f23f2956371b87dcc92c263ed5bb1e:

```suggestion
// For length 1 vectors, clone key providers until vector is the same length
```

👍 andrewtoth approved a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2222373701)
utACK 02f6a42be3dfc57998304801c0d909bfa96745e4

💬 mzumsande commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2272318013)
>The height in the metadata clearly tells us which height the hash is expected at. Not having the same hash at that height clearly tells us that the node is on a different chain than was expected by the metadata. That is better than just saying "we don't know this hash for whatever reason".

Ah, ok, my point is that the height is implied by the block hash, together with the block index db of the node. If the block tree db doesn't contain a header with that block hash, then we abort anyway. If
...

💬 achow101 commented on pull request "miniscript: Use `ToIntegral` instead of `ParseInt64`":
(https://github.com/bitcoin/bitcoin/pull/30577#issuecomment-2272366647)
ACK 6714276d72244c2e2dbe9617f1341ba7fc06c0cc

🚀 achow101 merged a pull request: "miniscript: Use `ToIntegral` instead of `ParseInt64`"
(https://github.com/bitcoin/bitcoin/pull/30577)

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706262457)
Done, since I'm making changes anyways.

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706262514)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706262593)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1706262619)
Done

💬 tdb3 commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1704667895)
nitty nit: If this file ends up being changed again, could remove the extra space between the first `%s` and the newline.

👍 tdb3 approved a pull request: "doc, rpc : `#30275` followups"
(https://github.com/bitcoin/bitcoin/pull/30525#pullrequestreview-2219921393)
re ACK fa2f26960ee084971ab09959b213a9b8104482e5

🤔 tdb3 reviewed a pull request: "docs: doc update for mempoolfullrbf default + log deprecation"
(https://github.com/bitcoin/bitcoin/pull/30594#pullrequestreview-2222478599)
Approach ACK

💬 tdb3 commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706274236)
Maybe we can use `LogInfo` here?

🚀 ryanofsky merged a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051)

💬 achow101 commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#issuecomment-2272523519)
ACK fa2f26960ee084971ab09959b213a9b8104482e5

💬 ismaelsadeeq commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1706382050)
Thank you.
I will address this and @willcl-ark nit if there is need to retouch.

🚀 achow101 merged a pull request: "doc, rpc : `#30275` followups"
(https://github.com/bitcoin/bitcoin/pull/30525)