💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467640407)
Yeah, this was meant to be a class encapsulating all sqlite functions so that we can trigger different errors and improve and test our error-handling code. I'm not sure why I implemented `CanExecute` instead of decoupling the `Exec()`, but, well, I just changed the implementation to follow the initial idea. Thanks for pointing this out.

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467634537)
upps thanks. Fixed.

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467629362)
done

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467631981)
Sure, done. Have slightly rephrased the suggestion. Thanks

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467629193)
Sure, done. Comment added.

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467644011)
So true that it hurts.
I tend to call the `WalletBatch` handler because 'batch' refers to a class that groups operations and dumps the result all at once atomically at the end of the process. This is something that the `WalletBatch` class doesn't do unless we explicitly begin and finish the transaction.

👋 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.
...