🤔 theStack reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-1845552797)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-1845552797)
Concept ACK
💬 theStack commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1467521980)
IIUC, the right part of this expression seems redundant?
```suggestion
self._requires_wallet = self.is_wallet_compiled()
```
If a wallet is compiled, that implies that either SQLite or BDB is compiled as well.
Consequently, the `self.has_wallet` member is not needed anymore, and can simply be replaced by `self.is_wallet_compiled()` calls?
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1467521980)
IIUC, the right part of this expression seems redundant?
```suggestion
self._requires_wallet = self.is_wallet_compiled()
```
If a wallet is compiled, that implies that either SQLite or BDB is compiled as well.
Consequently, the `self.has_wallet` member is not needed anymore, and can simply be replaced by `self.is_wallet_compiled()` calls?
💬 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.
(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.
(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.
(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.
(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.
(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
(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
(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.
(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.
(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)
(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?
(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.
(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
(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.
(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.
(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.
(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
(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
...
(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
...