💬 TheCharlatan commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1666788309)
ACK 1c976c691cc4b20f43071aabf36c7afed1571057

💬 MarcoFalke commented on issue "Flaky `wallet_transactiontime_rescan.py --legacy-wallet` functional test":
(https://github.com/bitcoin/bitcoin/issues/28221#issuecomment-1666789562)
Looks like it did got dispatched to thread 2, but thread 0 was blocking for 24 minutes:

```
test 2023-08-04T17:19:25.474000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=encrypted_wallet', 'walletpassphrase', 'passphrase', '1']
node0 2023-08-04T17:19:25.478233Z (mocktime: 2023-09-03T17:11:37Z) [http] [httpserver.cpp:254] [http_request_cb] [http] Received a POST request for /wallet/encrypted_wallet from 127.0.0.1:56776
node0 2023-08-04T17:19:25.478352Z (mocktime:
...

💬 MarcoFalke commented on issue "Flaky `wallet_transactiontime_rescan.py --legacy-wallet` functional test":
(https://github.com/bitcoin/bitcoin/issues/28221#issuecomment-1666789809)
Probably an upstream GCE outage?

💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1285185051)
I wonder why implicit fee estimation passed in the first place. Previously there were two inputs and a fee of 550sat. Now there is a fee of 2200 sat with one input.

Mind creating a pull, as I can't test intermittent issues? I think this needs clarification/fixup even absent an intermittent issue.

💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1666799645)
> This will remove Cirrus from the loop entirely

On a second thought, I don't think this is possible. We use Cirrus CI compute credits to run the linters *immediately*, which take a few seconds. I don't think there is another CI provider that offers this feature, and I think this feature is well worth it to pay for (usually USD$ ~0.01 per run)

💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666801219)
> This propaganda needs to stop. OP_RETURN is not bad. In case you forgot it's inspcriptions that have led to the largest blocks ever on bitcoin, not bare p2sh.
>
> If we followed the logic proposed in your response we should also roll back taproot and replace the limit on the script sigs. Spam from taproot lifting the limit on scriptsigs far exceeds the legitimate uses of all p2sh usage. Lifting the script sig without validating the input is a bug no one wants to admit exists. I can decode a
...

💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1285189804)
Thanks, good catch. Fixed typo.

💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1285189851)
v1=false means v2=true, which is what you are asking for and what this pull request is doing, no?

🤔 zkfrio requested changes to a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217#pullrequestreview-1564119825)
NACK

After reading recent [discussion](https://github.com/bitcoin/bitcoin/pull/27578) about standardness, I think we should relax some standardness rules in Bitcoin Core if P2P has to remain relevant for transaction relay of a censorship resistant network.

Even if `false` is made default in next version, there will be some nodes and miners allowing bare multisig and that's all a normal user needs to use bare multisig.

💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666812614)
@zkfrio your statement is contradictory. If bare multisig inscribers can easily route around mempool policy there's no reason to oppose the change. If you ACK this proposal everyone can have their slice of pie.

💬 Nyanraltotlapun commented on issue "Does not use bind to local address for outgoing connections":
(https://github.com/bitcoin/bitcoin/issues/6476#issuecomment-1666817316)
Well, I still interested in it. It is a logical thing to do in general.
I think people uses some workarounds for this when they need this, but have native behavior is right thing to do.

💬 vostrnad commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666832908)
I ran the numbers on bare multisig usage between blocks 700,000 and 800,000 (approx. last two years). Because most of bare multisig activity is never spent outputs, I focused on bare multisig inputs and how old the spent coins are.

There were 940 bare multisig inputs during the analyzed period. 28% of them were spending coins created in the same block, while the median coin age was 224 blocks, and 90% of the coins were 14,599 blocks or fewer old. So while the actual usage of bare multisig for
...

💬 martinus commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-1666842501)
I think most of the slowdown is due to the locking required by each `fread()` call. I did a quick benchmark by switching back to `CAutoFile` and and using `fread_unlocked` instead of `std::fread` (and call `flockfile` in the ctor and `funlockfile` before `fclose`), and the performance is practically the same as with `CBufferedFile`.

I don't mind dropping the change to `CBufferedFile`, but I think the `TRACE_RAII` macro is nice (maybe with a better name, I couldn't think of anything else...).
...

💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285205986)
(in commit 2de8e8309ac0ee60742e45b8cc1342e61c725566)
nit: could be written as one-liner
```suggestion
return (it != m_selected.end() && it->second.HasTxOut());
```
(same for `HasInputWeight`, though that gets replaced anyway in the next commit)

💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285212353)
typo, I guess
```suggestion
// If an input has a preset sequence, we can't do anti-fee-sniping
```

💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285210302)
(in commit 077783d24b7f1e3d3312e6cc12aee3f178400725)
Looking at the "override calculated size with coin control specified size" branch removed below, seems like a `GetVirtualTransactionSize` call is missing here (in the case that `GetInputWeight` doesn't return std::nullopt), as we want vbytes as `input_bytes` unit, not WU?

💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285213341)
(in commit 6fcbf9f44358f85aab4c0b6617c113b4c37da5a8)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end() && it->second.HasScripts());
```

💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285211710)
(in commit 705568146d4399ad8089adc4943786e2a0c0a62e)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end()) ? it->second.GetSequence() : std::nullopt;
```

💬 Semisol commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666868952)
Concept ACK

I do not see the negative impact of this change, as there is a heavy incentive to use P2WSH or P2SH due to the cost savings and/or wallets not being able to send to bare multisigs easily.

This also has the additional benefit of encouraging adoption of more widely supported standards and preventing spam.

💬 ns-xvrn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666885693)
Concept ACK

For the reasoning mentioned by achow101 and semisol. Avoiding spam is healthy for my full node.