💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177915783)
As i am retouching i re-read this commit message and i didn't find any such typo. Could you be more precised? (Although i don't plan on addressing if i don't retouch again.)
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177915783)
As i am retouching i re-read this commit message and i didn't find any such typo. Could you be more precised? (Although i don't plan on addressing if i don't retouch again.)
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024553755)
> a PR that restricts default policy should have a corresponding post on the mailing list
Indeed.
> The primary motivation should be the same as what's in BIP 54: to avoid long validation times.
That's not necessarily true though. Transactions that exceed this limit, but are still below the maximum standardness size, are very expensive to make. It's not obvious that such an expense is worth it just to slow down a few nodes on the network once. But once the proposed soft fork is active,
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024553755)
> a PR that restricts default policy should have a corresponding post on the mailing list
Indeed.
> The primary motivation should be the same as what's in BIP 54: to avoid long validation times.
That's not necessarily true though. Transactions that exceed this limit, but are still below the maximum standardness size, are very expensive to make. It's not obvious that such an expense is worth it just to slow down a few nodes on the network once. But once the proposed soft fork is active,
...
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177922559)
> In addition, this limit may only be run into by pathological
transactions which pad the Script with sigops but do not use actual signatures when spending, as
otherwise they would run into the standard transaction size limit.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177922559)
> In addition, this limit may only be run into by pathological
transactions which pad the Script with sigops but do not use actual signatures when spending, as
otherwise they would run into the standard transaction size limit.
💬 maflcko commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2177924690)
missed this?
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2177924690)
missed this?
🤔 pablomartin4btc reviewed a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2975993807)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2975993807)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
💬 pablomartin4btc commented on pull request "doc: update unix build doc with build flags":
(https://github.com/bitcoin/bitcoin/pull/32269#discussion_r2177928251)
I do agree but that's consistent with most `doc/build*.md` files (`doc/build-freebsd.md`, `doc/build-netbsd.md`, `doc/build-openbsd.md`, `doc/build-osx.md`).
(https://github.com/bitcoin/bitcoin/pull/32269#discussion_r2177928251)
I do agree but that's consistent with most `doc/build*.md` files (`doc/build-freebsd.md`, `doc/build-netbsd.md`, `doc/build-openbsd.md`, `doc/build-osx.md`).
💬 hebasto commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3024565853)
This also is related: https://codeberg.org/guix/guix/issues/556.
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3024565853)
This also is related: https://codeberg.org/guix/guix/issues/556.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177930662)
Sure, makes sense.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177930662)
Sure, makes sense.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177930774)
Right, the witnesses are never inspected by `AreInputsStandard`. Dropped the unnecessary signing logic, to get a neat 13 lines reduction in the diff.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177930774)
Right, the witnesses are never inspected by `AreInputsStandard`. Dropped the unnecessary signing logic, to get a neat 13 lines reduction in the diff.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024571708)
@glozow
I agree this needs a ML post. Will take care of this tomorrow.
However the motivation for this PR is not to reduce validation times, as it only marginally reduces the worst case for standard transactions. I don't think it's fair to qualify the motivation given in OP of "presumptuous" and it's certainly not a secondary motivation: if a soft fork is activated and such transactions are still standard it would be trivial to DoS any unupgraded miner into producing a block invalid accord
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024571708)
@glozow
I agree this needs a ML post. Will take care of this tomorrow.
However the motivation for this PR is not to reduce validation times, as it only marginally reduces the worst case for standard transactions. I don't think it's fair to qualify the motivation given in OP of "presumptuous" and it's certainly not a secondary motivation: if a soft fork is activated and such transactions are still standard it would be trivial to DoS any unupgraded miner into producing a block invalid accord
...
💬 hebasto commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3024595350)
> I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't work.
FWIW, on some systems, such as [NetBSD](https://github.com/hebasto/bitcoin-core-nightly/blob/a1f9ee0d675cf74bf5caaec1131800c5d2b66893/.github/workflows/netbsd.yml#L127C21-L127C153), it may be necessary to be more specific about the compilers used to build native packages:
```
gmake -C depends MULTIPROCESS=1 build_CC=/usr/pkg/gcc14/bin/gcc build_CXX=/usr/pkg/gcc14/bin/g++ CC=/usr/pkg/gcc14/bi
...
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3024595350)
> I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't work.
FWIW, on some systems, such as [NetBSD](https://github.com/hebasto/bitcoin-core-nightly/blob/a1f9ee0d675cf74bf5caaec1131800c5d2b66893/.github/workflows/netbsd.yml#L127C21-L127C153), it may be necessary to be more specific about the compilers used to build native packages:
```
gmake -C depends MULTIPROCESS=1 build_CC=/usr/pkg/gcc14/bin/gcc build_CXX=/usr/pkg/gcc14/bin/g++ CC=/usr/pkg/gcc14/bi
...
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024600108)
re-ACK 65b7deda12338b17f342c1d37f44e7c6da850771
([more future](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177518858), [less signing](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175236097))
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024600108)
re-ACK 65b7deda12338b17f342c1d37f44e7c6da850771
([more future](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177518858), [less signing](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175236097))
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177952812)
Good to know you are verifying my findings. I hadn't seen your message before leaving my comment, looks like you already confirmed the first one, good.
Regarding how expensive transaction processing can be, yes this is interesting and something i have spent a fair bit of time looking into. However it's fairly tangential to this PR so let's discuss it separately. I DM'd you about this elsewhere.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177952812)
Good to know you are verifying my findings. I hadn't seen your message before leaving my comment, looks like you already confirmed the first one, good.
Regarding how expensive transaction processing can be, yes this is interesting and something i have spent a fair bit of time looking into. However it's fairly tangential to this PR so let's discuss it separately. I DM'd you about this elsewhere.
💬 maflcko commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178007144)
KeyError: 'immature_balance'
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178007144)
KeyError: 'immature_balance'
💬 pablomartin4btc commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178024826)
It was removed in #32721.
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178024826)
It was removed in #32721.
👍 theStack approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976158406)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
Makes sense to clarify, have tripped over this as well in the past.
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976158406)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
Makes sense to clarify, have tripped over this as well in the past.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178048277)
Done! Also added the empty array case to the test and updated the description in before & after sections with it.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178048277)
Done! Also added the empty array case to the test and updated the description in before & after sections with it.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2178057885)
I think the benchmark is fine as it stands, for the purpose it serves (which is not to measure transaction processing time). I don't think more needs to be done here and will resolve this thread.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2178057885)
I think the benchmark is fine as it stands, for the purpose it serves (which is not to measure transaction processing time). I don't think more needs to be done here and will resolve this thread.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3024771408)
<ins>_Updates_</ins>:
- Addressed @stickies-v's feedback correcting the error condition for `getdescriptoractivity`, adding also the empty array case in its corresponding test and updating the PR description in the before & after sections accordingly.
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3024771408)
<ins>_Updates_</ins>:
- Addressed @stickies-v's feedback correcting the error condition for `getdescriptoractivity`, adding also the empty array case in its corresponding test and updating the PR description in the before & after sections accordingly.
💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3024775910)
Apologies for the force push. However, it seems that the fuzz target actually produces valid block headers, so the chainman state is dirty and needs to be reset as well for those fuzz inputs. I've pushed a new commit doing this (after a rebase).
There is still non-determinism in `operator==<transaction_identifier>`, however I fail to see the source. I may take a look in the future.
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3024775910)
Apologies for the force push. However, it seems that the fuzz target actually produces valid block headers, so the chainman state is dirty and needs to be reset as well for those fuzz inputs. I've pushed a new commit doing this (after a rebase).
There is still non-determinism in `operator==<transaction_identifier>`, however I fail to see the source. I may take a look in the future.