💬 michaelfolkson commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912071823)
@GregTonoski: So what are you using to do fee estimation and assessing what fee you need to pay to get your transaction into an upcoming block? I assume you are using someone else's mempool (e.g. whatever block explorer you use). This is fine, you are free to do this and many do. But other people would like the option of assessing fees using the mempool of their own full node and not relying on a centralized third party's mempool.

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1467634309)
Sure. Created a constant for the -999 error in the new approach.

🤔 furszy reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1845717595)
Updated per feedback, thanks @ryanofsky!

The main modification resides in the `ExecHandler` class, which is now designed to encapsulate all sqlite functions. This enables us to trigger different errors across the database sources; letting us improve the error-handling code and expand its test coverage.
Furthermore, future encapsulation of sqlite functions within this class will contribute to minimizing the impact of any future API changes.

💬 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`?