👋 fanquake's pull request is ready for review: "ci: Use LLVM 17.0.6 & DEBUG=1 in depends for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27495)

💬 fanquake commented on pull request "ci: Use LLVM 17.0.6 & DEBUG=1 in depends for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27495#issuecomment-1912087694)
I'm seeing multiple other issues when switching over to LLVM 18. Reverted to 17.0.6 for now. I think we could make this change to re-enable `DEBUG=1`, and follow up with LLVM 18 in future?

💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912104676)
> @tromp : you're probably the least qualified to talk about "defending an exploit", you created Grin cryptocurrency where people lost all their money

@knostr : please keep baseless ad-hominem attacks out of the discussion.

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1912105564)
Rebased https://github.com/bitcoin/bitcoin/commit/e0ea52da97b5bb18b813e862455aafd56a7809dd -> https://github.com/bitcoin/bitcoin/commit/08b62764190a907c0d7d329f4df07f9a2b6dcdeb

* Fixed small `configure.ac` merge conflict

💬 furszy commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1467681522)
> If a wallet is compiled, that implies that either SQLite or BDB is compiled as well.

Yeah sure. Removed the right part of the statement.

> Consequently, the self.has_wallet member is not needed anymore, and can simply be replaced by self.is_wallet_compiled() calls?

Sure. Done.

🤔 furszy reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-1845803551)
Updated per feedback. Thanks theStack.

📝 fanquake opened a pull request: "fuzz: also set MSAN_SYMBOLIZER_PATH"
(https://github.com/bitcoin/bitcoin/pull/29327)
Should resolve: https://github.com/bitcoin-core/qa-assets/issues/167.

👍 dergoegge approved a pull request: "fuzz: also set MSAN_SYMBOLIZER_PATH"
(https://github.com/bitcoin/bitcoin/pull/29327#pullrequestreview-1845806166)
utACK cf937b2068dba167b6c7ea6f8ee9ba366803c3bb

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1912123265)
Updated https://github.com/bitcoin/bitcoin/commit/08b62764190a907c0d7d329f4df07f9a2b6dcdeb -> https://github.com/bitcoin/bitcoin/commit/cbb8d3a6574e10841c52716bbcd27e0c60752e1c ([implement-bip352-01-rebase](https://github.com/josibake/bitcoin/tree/implement-bip352-01-rebase) -> [implement-bip352-02](https://github.com/josibake/bitcoin/tree/implement-bip352-02) , [compare](https://github.com/josibake/bitcoin/compare/implement-bip352-01-rebase..implement-bip352-02))

* Added doxygen style commen
...

👍 shaavan approved a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845820349)
Code Review ACK 8c067ef1c67a1053127a10b6312bcf71da446445

Notes:

- The Unserialise function first reads the compression flag from the stream and then updates the actual keydata.
- The Serialise function first writes the compression flag, followed by the keydata
- The test appropriately verifies the added code for both the compressed and uncompressed keys.

🤔 maflcko reviewed a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845873682)
How is this different from `CPrivKey`?

💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467728211)
nit: In C++, a `for` loop can be used to execute the same code block with different values.

💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467729703)
Not sure. IIUC the assumption is that keydata should not hold invalid keys. Can you explain why this assumption should be broken?

💬 theStack commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1467741563)
Have to admit that I'm lacking knowledge here about the concrete differences between `FillableSigningProvider` and `FlatSigningProvider`, especially about the possible benefits of the latter in this PR. AFAIR, I chose the fillable provider as it provides a nice interface (`.AddKey()`), which the flat one doesn't. The following patch works:
```diff
diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
index 8cb5e63882..8bd2ecbcd0 100644
--- a/src/bench/sign_transaction.
...

🤔 sdaftuar reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1845786642)
I haven't yet reviewed the tests -- still plan to do that.

FYI, despite my earlier comment suggesting we resurrect #27018, after giving this further thought I realized we don't need to worry about the issue of 0-fee transactions in the mempool at all until we get to the point that we're doing package relay/validation from the p2p network, and potentially even then we may not need such behavior (as there's just a free-relay concern that is easy to mitigate). So in the interests of narrowing t
...

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467745903)
Yes I think this can be scrapped now -- `ApplyV3Rules` only operates on a single transaction and its in-mempool parents; correct validation for packages should now all happen in `PackageV3Checks`.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467692141)
My view is that we'll end up reworking things substantially in order to enable package RBF, but I agree with the code comment suggestion.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467676423)
Agreed, that makes sense. Something like
```
bool has_mempool_sibling{false};
...
has_mempool_sibling = (parent->GetCountWithDescendants() > 1);
```
?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467674203)
For consistency with the rest of the code relating to the v3 rules, perhaps we should use the V3_ANCESTOR_LIMIT variable here, and use the `static_assert` like in `ApplyV3Rules` to indicate to code readers that this only works because the limit is 1?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467759890)
@sdaftuar do you mean for https://github.com/bitcoin/bitcoin/pull/28984 or for generalized package RBF? I'm thinking shorter term here.