💬 maflcko commented on issue "ci: failure in feature_index_prune.py (MSVC)":
(https://github.com/bitcoin/bitcoin/issues/29326#issuecomment-1912061205)
Duplicate of #29090?

🤔 shaavan reviewed a pull request: "Add a `-permitbarepubkey` option"
(https://github.com/bitcoin/bitcoin/pull/29309#pullrequestreview-1845746439)
Concept ACK

<details>
<summary>
<b>Problems of P2PK:</b>
</summary>

1. **Security Risk**: Directly exposing the public key poses a risk if cryptographic standards change or are compromised.
2. **Larger Transaction Size**: P2PK transactions are larger due to the inclusion of full public keys, leading to higher transaction fees.
3. **Privacy Concerns**: Exposing public keys can compromise user privacy by allowing easier linkage of transactions.
</details>

Considering the flaws of P2
...

💬 shaavan commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#discussion_r1467647506)
nit: Should we mention P2PK here?

```suggestion
argsman.AddArg("-permitbarepubkey", strprintf("Relay legacy pubkey (P2PK) outputs (default: %u)", DEFAULT_PERMIT_BAREPUBKEY), ArgsManager::ALLOW_ANY,
```

✅ fanquake closed an issue: "ci: failure in feature_index_prune.py (MSVC)"
(https://github.com/bitcoin/bitcoin/issues/29326)

💬 fanquake commented on issue "ci: failure in feature_index_prune.py (MSVC)":
(https://github.com/bitcoin/bitcoin/issues/29326#issuecomment-1912064569)
Yea I think so. Was trying to remember which of the Windows issues documented this failure. Will move to there.

💬 fanquake commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-1912064796)
https://github.com/bitcoin/bitcoin/actions/runs/7667501768/job/20897463395

```bash
============
Combined log for D:\a\_temp/test_runner_₿_🏃_20240126_113602/feature_index_prune_293:
============

test 2024-01-26T11:36:28.165000Z TestFramework (INFO): PRNG seed is: 8852427722333217080
test 2024-01-26T11:36:28.165000Z TestFramework (DEBUG): Setting up network thread
test 2024-01-26T11:36:28.212000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_202
...

🤔 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

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

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