💬 maflcko commented on pull request "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING":
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155286213)
Induced failure works and looks as expected: https://github.com/bitcoin/bitcoin/runs/47420378191 (Just adding a new `Exit status: 137` at the end)
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155286213)
Induced failure works and looks as expected: https://github.com/bitcoin/bitcoin/runs/47420378191 (Just adding a new `Exit status: 137` at the end)
🤔 Jacksdad2015 reviewed a pull request: "[29.x] doc: minor rel notes changes"
(https://github.com/bitcoin/bitcoin/pull/32252#pullrequestreview-3088402937)
Don't know
(https://github.com/bitcoin/bitcoin/pull/32252#pullrequestreview-3088402937)
Don't know
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3155344377)
> Why would this case ever occur?
Most likely never because we have the detection. If we didn't, someone may try to exploit this case to hinder propagation of 1p1c packages (see OP).
> Seems like a lot of code to handle an unexpected path.
Yeah. Note however this is *replacing* expensive code. I think it's always fair to question adding more code, but i hope in this instance it is pretty clear that replacing the expensive code by an inexpensive, albeit slightly more verbose, one is wort
...
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3155344377)
> Why would this case ever occur?
Most likely never because we have the detection. If we didn't, someone may try to exploit this case to hinder propagation of 1p1c packages (see OP).
> Seems like a lot of code to handle an unexpected path.
Yeah. Note however this is *replacing* expensive code. I think it's always fair to question adding more code, but i hope in this instance it is pretty clear that replacing the expensive code by an inexpensive, albeit slightly more verbose, one is wort
...
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254489248)
Thx, but this reminds me that the setting the min port is missing? https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254489248)
Thx, but this reminds me that the setting the min port is missing? https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236668156
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254490171)
Actually, with `network=host`, this is needed, no?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2254490171)
Actually, with `network=host`, this is needed, no?
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254514636)
Hmm and if we do so, we don't even need to carve out an exception for P2A, because using P2A with an empty witness can never result in a failure.
I'm going to implement that now.
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254514636)
Hmm and if we do so, we don't even need to carve out an exception for P2A, because using P2A with an empty witness can never result in a failure.
I'm going to implement that now.
💬 pinheadmz commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3155500225)
concept ACK, tested the mainnet snapshot load with the QML gui branch built on top of this
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3155500225)
concept ACK, tested the mainnet snapshot load with the QML gui branch built on top of this
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254614655)
Right, i should have double checked. Fixed in upcoming push, thanks.
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254614655)
Right, i should have double checked. Fixed in upcoming push, thanks.
💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254632919)
Changing the call site to where we currently perform detection in master is not going to avoid all behaviour changes. Short of checking all consensus rules for all other inputs preceding the one that failed the check, we can't guarantee that we won't return a false-positive witness-stripped error. For instance if we receive a transaction with two inputs, the first a legacy P2PKH, the second a Segwit P2WPKH, then if the transaction comes in without witness but also fails the signature check for t
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2254632919)
Changing the call site to where we currently perform detection in master is not going to avoid all behaviour changes. Short of checking all consensus rules for all other inputs preceding the one that failed the check, we can't guarantee that we won't return a false-positive witness-stripped error. For instance if we receive a transaction with two inputs, the first a legacy P2PKH, the second a Segwit P2WPKH, then if the transaction comes in without witness but also fails the signature check for t
...
💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3155611529)
I still think this PR is preferable to #33105. The latter will inevitably introduce false-positive witness stripped errors, whereby we won't add some transactions with consensus/standardness errors to the reject filter. Since we already don't add all transactions with consensus/standardness errors to the reject filter, if we also don't care about these additional false positives, we might as well just to the cleaner thing and get rid of this filter entirely as is done here.
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3155611529)
I still think this PR is preferable to #33105. The latter will inevitably introduce false-positive witness stripped errors, whereby we won't add some transactions with consensus/standardness errors to the reject filter. Since we already don't add all transactions with consensus/standardness errors to the reject filter, if we also don't care about these additional false positives, we might as well just to the cleaner thing and get rid of this filter entirely as is done here.
💬 0xB10C commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155627414)
I've published a chart showing the share of sub 1 sat/vByte transactions on [mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions](https://mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions/).
I also generated a recent export of my [stats on compact block reconstructions](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052) over the last two months:
<img width="1093" height="584" alt="image" src="https://github.com/user-attachments/assets/fb15af92-325b-4669
...
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155627414)
I've published a chart showing the share of sub 1 sat/vByte transactions on [mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions](https://mainnet.observer/charts/fees-sub-1-sat-vbyte-transactions/).
I also generated a recent export of my [stats on compact block reconstructions](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052) over the last two months:
<img width="1093" height="584" alt="image" src="https://github.com/user-attachments/assets/fb15af92-325b-4669
...
💬 darosior commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155651614)
> In June, we were requesting less than 10 kB worth of transactions per block on average. Now, we are requesting close to 0.8 MB worth of transactions per block on average.
Crazy. Although this is a significant diff we might want to backport this PR at least to 29.
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3155651614)
> In June, we were requesting less than 10 kB worth of transactions per block on average. Now, we are requesting close to 0.8 MB worth of transactions per block on average.
Crazy. Although this is a significant diff we might want to backport this PR at least to 29.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254641829)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (37c1d8fc8db2f1ddd6ddf4d8fccbd13bbc9c9935)
Two things:
- It might be nice to make `CountP2SHSigOps` function more similar to `CountWitnessSigOps` and accept a `scriptPubKey` argument. It looks like this change also makes call sites simpler.
- I noticed with BIP54 the `CScript::CountSigOps` comment when to use `fAccurate=false` will not be right anymore. Would suggest qualifying the comment with "When enforcing the `MAX_BLOCK_SIGO
...
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254641829)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (37c1d8fc8db2f1ddd6ddf4d8fccbd13bbc9c9935)
Two things:
- It might be nice to make `CountP2SHSigOps` function more similar to `CountWitnessSigOps` and accept a `scriptPubKey` argument. It looks like this change also makes call sites simpler.
- I noticed with BIP54 the `CScript::CountSigOps` comment when to use `fAccurate=false` will not be right anymore. Would suggest qualifying the comment with "When enforcing the `MAX_BLOCK_SIGO
...
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254692162)
In commit "refactor: pull `IsPayToTaproot` to header" (38e89f646221ec0c15158b6ef0007336fec008e3)
Similar to previous, could replace `34` with `WITNESS_V1_TAPROOT_SIZE + 2` but no strong preference
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254692162)
In commit "refactor: pull `IsPayToTaproot` to header" (38e89f646221ec0c15158b6ef0007336fec008e3)
Similar to previous, could replace `34` with `WITNESS_V1_TAPROOT_SIZE + 2` but no strong preference
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254707312)
In commit "refactor: add `IsPayToPubKeyHash` helper to script.h" (5d23e9a4c6fa4aab530889c54565954486572afd)
Again it seems a little off to use a segwit constant in pre-segwit output. Might be better to stick with literal 20 or maybe introduce a HASH160_SIZE or PUBKEY_HASH_SIZE constant. No strong preference though and technically this should be ok.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254707312)
In commit "refactor: add `IsPayToPubKeyHash` helper to script.h" (5d23e9a4c6fa4aab530889c54565954486572afd)
Again it seems a little off to use a segwit constant in pre-segwit output. Might be better to stick with literal 20 or maybe introduce a HASH160_SIZE or PUBKEY_HASH_SIZE constant. No strong preference though and technically this should be ok.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254675962)
In commit "refactor: pull `IsPayToWitnessScriptHash` to header" (3c800a72b6520d94bc3996fa31ecded0f6fab22d)
Changing `34` to `WITNESS_V0_SCRIPTHASH_SIZE + 2` might make this a little more self-documenting.
(I don't mean to bikeshed on use of literals vs constants and front/back in these functions though. I could imagine other developers preferring the literals for concreteness even if I like the constants, and I don't have a strong preference.)
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254675962)
In commit "refactor: pull `IsPayToWitnessScriptHash` to header" (3c800a72b6520d94bc3996fa31ecded0f6fab22d)
Changing `34` to `WITNESS_V0_SCRIPTHASH_SIZE + 2` might make this a little more self-documenting.
(I don't mean to bikeshed on use of literals vs constants and front/back in these functions though. I could imagine other developers preferring the literals for concreteness even if I like the constants, and I don't have a strong preference.)
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254731153)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
Seems reasonable to include pubkey.h but since we are only using constants, it might be better to move constants somewhere.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254731153)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
Seems reasonable to include pubkey.h but since we are only using constants, it might be better to move constants somewhere.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254716286)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
I guess it is intentional to replace `CPubKey::COMPRESSED_SIZE + 2` with 35 here?
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254716286)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
I guess it is intentional to replace `CPubKey::COMPRESSED_SIZE + 2` with 35 here?
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254732750)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
Might be good to document that this method does not check the pubkey prefix (even/odd/uncompressed) and verify this is even or odd.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254732750)
In commit "refactor: add `IsCompressedPayToPubKey` helper to script.h" (017f58821fe857a2bb7ac14f2b00b349936391d8)
Might be good to document that this method does not check the pubkey prefix (even/odd/uncompressed) and verify this is even or odd.
✅ maflcko closed a pull request: "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING"
(https://github.com/bitcoin/bitcoin/pull/33137)
(https://github.com/bitcoin/bitcoin/pull/33137)
💬 maflcko commented on pull request "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING":
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155890738)
No idea why neither kill nor setsid works on macos, but both work on Linux. I guess this means `CI_FAILFAST_TEST_LEAVE_DANGLING` is still needed.
(https://github.com/bitcoin/bitcoin/pull/33137#issuecomment-3155890738)
No idea why neither kill nor setsid works on macos, but both work on Linux. I guess this means `CI_FAILFAST_TEST_LEAVE_DANGLING` is still needed.