💬 MarcoFalke commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-1666759670)
Shouldn't `std::fread` be already cached? I could imagine the result to be different on different hardware? Also, `-reindex-chainstate` seems rare enough to not spend time to optimize its runtime? Going further, if the only goal of `CBufferedFile` is to speed up `-reindex` by 1%, we could consider removing it. Since the implementation is put in the header file of `streams.h`, removing it could reduce peak compile memory usage by .5% and compilation speed by .3% on every fresh compilation IIRC. O
...
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-1666759670)
Shouldn't `std::fread` be already cached? I could imagine the result to be different on different hardware? Also, `-reindex-chainstate` seems rare enough to not spend time to optimize its runtime? Going further, if the only goal of `CBufferedFile` is to speed up `-reindex` by 1%, we could consider removing it. Since the implementation is put in the header file of `streams.h`, removing it could reduce peak compile memory usage by .5% and compilation speed by .3% on every fresh compilation IIRC. O
...
💬 MarcoFalke commented on issue "fuzz:crc32c::ExtendArm64 Undefined symbols for architecture arm64:":
(https://github.com/bitcoin/bitcoin/issues/28223#issuecomment-1666762164)
Closing for now. Let us know if this is still an issue or if you have any other questions.
(https://github.com/bitcoin/bitcoin/issues/28223#issuecomment-1666762164)
Closing for now. Let us know if this is still an issue or if you have any other questions.
✅ MarcoFalke closed an issue: "fuzz:crc32c::ExtendArm64 Undefined symbols for architecture arm64:"
(https://github.com/bitcoin/bitcoin/issues/28223)
(https://github.com/bitcoin/bitcoin/issues/28223)
💬 MarcoFalke commented on pull request "Bump python minimum version to 3.9":
(https://github.com/bitcoin/bitcoin/pull/28211#issuecomment-1666771306)
> Isn't Stream a rolling release? What about RHEL?
I don't think Stream it is a rolling release. It should ship all the same package *versions* as the corresponding RHEL release (however, the binaries may not be bit-by-bit-identical).
I've adjusted the pull request description to say `CentOS-like 8/9`.
You can verify it locally by running a fresh install of CentOS 9 Stream / Alma Linux 9 / Rocky Linux 9 and typing `python3 --version`. For me, it gives: `Python 3.9.17`. Trying to install
...
(https://github.com/bitcoin/bitcoin/pull/28211#issuecomment-1666771306)
> Isn't Stream a rolling release? What about RHEL?
I don't think Stream it is a rolling release. It should ship all the same package *versions* as the corresponding RHEL release (however, the binaries may not be bit-by-bit-identical).
I've adjusted the pull request description to say `CentOS-like 8/9`.
You can verify it locally by running a fresh install of CentOS 9 Stream / Alma Linux 9 / Rocky Linux 9 and typing `python3 --version`. For me, it gives: `Python 3.9.17`. Trying to install
...
💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1666779214)
> (Although downgrading might be something we want to support?)
It is. You can simply set `-persistmempoolv1=1`.
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1666779214)
> (Although downgrading might be something we want to support?)
It is. You can simply set `-persistmempoolv1=1`.
💬 TheCharlatan commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1666788309)
ACK 1c976c691cc4b20f43071aabf36c7afed1571057
(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:
...
(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?
(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.
(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)
(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
...
(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.
(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?
(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.
(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.
(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.
(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
...
(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...).
...
(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)
(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
```
(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
```