💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656240907)
Sent a mail to Jess from the Bitcoin Defense Legal Fund to collect more legal opinions with the context. Normally luke-jr (luke@dashjr.org) and jonatack (jon@atack.com) are cc.
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656240907)
Sent a mail to Jess from the Bitcoin Defense Legal Fund to collect more legal opinions with the context. Normally luke-jr (luke@dashjr.org) and jonatack (jon@atack.com) are cc.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656279094)
> https://duckduckgo.com/?q=chattr+inside+docker should fix it
We're still having trouble getting this to work:
https://github.com/achow101/bitcoin/commits/28171-chattr-fixes
https://cirrus-ci.com/build/5095863295410176
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656279094)
> https://duckduckgo.com/?q=chattr+inside+docker should fix it
We're still having trouble getting this to work:
https://github.com/achow101/bitcoin/commits/28171-chattr-fixes
https://cirrus-ci.com/build/5095863295410176
🤔 Sjors reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552855193)
Also reviewed a733dd79e29068ad1e0532ac42a45188a040a7b9, d4a11abb1972b54f0babdccfbb2fde97ab885933, 3556b850221bc0e597d7dd749d4d47ab58dc8083 and read through all of `CheckBlockIndex()`. Will continue later (going mostly in reverse order).
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552855193)
Also reviewed a733dd79e29068ad1e0532ac42a45188a040a7b9, d4a11abb1972b54f0babdccfbb2fde97ab885933, 3556b850221bc0e597d7dd749d4d47ab58dc8083 and read through all of `CheckBlockIndex()`. Will continue later (going mostly in reverse order).
💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278023724)
3556b850221bc0e597d7dd749d4d47ab58dc8083: if you have to retouch, can you add a comment that `!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip())` means "pindex has more work than the tip or was received earlier"
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278023724)
3556b850221bc0e597d7dd749d4d47ab58dc8083: if you have to retouch, can you add a comment that `!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip())` means "pindex has more work than the tip or was received earlier"
💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278035016)
3556b850221bc0e597d7dd749d4d47ab58dc8083: there's a tie breaker in the comparator for if two blocks with equal work were loaded from disk. Presumably that only matters if you restart right during a reorg in some weird way.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278035016)
3556b850221bc0e597d7dd749d4d47ab58dc8083: there's a tie breaker in the comparator for if two blocks with equal work were loaded from disk. Presumably that only matters if you restart right during a reorg in some weird way.
💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1656289233)
@naumenkogs I added one more commit that limits the number of simultaneous forced-inbound connections to 8. I didn't add it as an init option yet but we could if you like that approach. A connection only counts as forced-inbound if it succeeded in evicting another peer, and that is marked by a new bool in `CNodeOptions`
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1656289233)
@naumenkogs I added one more commit that limits the number of simultaneous forced-inbound connections to 8. I didn't add it as an init option yet but we could if you like that approach. A connection only counts as forced-inbound if it succeeded in evicting another peer, and that is marked by a new bool in `CNodeOptions`
💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656327982)
ACK 19b6353d3866769ea84b7759396078745c0a8ecf
Although the `chattr` approach worked locally, we couldn't figure out how to make it work in CI, so the approach of running CI as a non-root user seems reasonable.
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656327982)
ACK 19b6353d3866769ea84b7759396078745c0a8ecf
Although the `chattr` approach worked locally, we couldn't figure out how to make it work in CI, so the approach of running CI as a non-root user seems reasonable.
💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1278078208)
> It is an undocumented requirement that the functional usdt tests are run as root (not sure why).
For whatever reason, USDTs don't work for non-root users.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1278078208)
> It is an undocumented requirement that the functional usdt tests are run as root (not sure why).
For whatever reason, USDTs don't work for non-root users.
💬 luke-jr commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1278081971)
I mentioned these specifically because:
1. ChatGPT is the most popularly known, and most likely to be searched for if someone is considering using it.
2. GitHub promotes use of Copilot heavily, and we are using GitHub.
3. Meta is falsely advertising LLaMA as open source, and many people are just believing that without verifying. (The source code is not available, and the license is not permissive)
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1278081971)
I mentioned these specifically because:
1. ChatGPT is the most popularly known, and most likely to be searched for if someone is considering using it.
2. GitHub promotes use of Copilot heavily, and we are using GitHub.
3. Meta is falsely advertising LLaMA as open source, and many people are just believing that without verifying. (The source code is not available, and the license is not permissive)
💬 achow101 commented on pull request "script: check op_verif and op_vernotif":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656430144)
Concept ACK
I agree that `BAD_OPCODE` is the wrong error for these opcodes. However, instead of adding a new opcode, I think these should just return the existing `DISABLED_OPCODE` error that is used for the other disabled opcodes.
Also `OP_VER` should be included here as well as that's really what is disabled. `OP_VERIF` and `OP_VERNOTIF` are really just variants of `OP_VER`.
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656430144)
Concept ACK
I agree that `BAD_OPCODE` is the wrong error for these opcodes. However, instead of adding a new opcode, I think these should just return the existing `DISABLED_OPCODE` error that is used for the other disabled opcodes.
Also `OP_VER` should be included here as well as that's really what is disabled. `OP_VERIF` and `OP_VERNOTIF` are really just variants of `OP_VER`.
💬 Dearfor commented on pull request "script: check op_verif and op_vernotif":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656471607)
Ok thanks.
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1656471607)
Ok thanks.
🤔 furszy reviewed a pull request: "wallet: bugfix, disallow migration of invalid scripts"
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1553154514)
Feedback tackled, thanks achow101.
Expanded test coverage with the requested label check and few more checks. Now the test:
1) Verifies that neither the invalid double sh script label, nor the `addr(<script_hash_addr>)` descriptor, are contained by any of the migrated wallets.
2) Verifies that the valid sh script (the original single sh script that is imported at the same time as the invalid one) and its address book record are contained by the migrated watch-only wallet.
Also, while was
...
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1553154514)
Feedback tackled, thanks achow101.
Expanded test coverage with the requested label check and few more checks. Now the test:
1) Verifies that neither the invalid double sh script label, nor the `addr(<script_hash_addr>)` descriptor, are contained by any of the migrated wallets.
2) Verifies that the valid sh script (the original single sh script that is imported at the same time as the invalid one) and its address book record are contained by the migrated watch-only wallet.
Also, while was
...
💬 furszy commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1278210618)
> Perhaps check that this label doesn't appear in any of the migrated wallets?
Sure, expanded the test to cover it, and also added few other additional cases.
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1278210618)
> Perhaps check that this label doesn't appear in any of the migrated wallets?
Sure, expanded the test to cover it, and also added few other additional cases.
💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1278215639)
So if my understanding of the API is correct, you receive a package from `AcceptPackage()`, and there is a call to the constructor `AncestorPackage()`, then we verify each transaction being component of the package is in the mempool by wtxid or txid or `PreChecks()` them.
If the transaction is `PreChecks()` valid, then we call `AddFeeAndVsize()`. If we have a `TX_SINGLE_FAILURE`, we add `AddFeeAndVsize()` or if we have a `TX_MISSING_INPUTS`, we `SkipWithDescendants()` the transaction. All oth
...
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1278215639)
So if my understanding of the API is correct, you receive a package from `AcceptPackage()`, and there is a call to the constructor `AncestorPackage()`, then we verify each transaction being component of the package is in the mempool by wtxid or txid or `PreChecks()` them.
If the transaction is `PreChecks()` valid, then we call `AddFeeAndVsize()`. If we have a `TX_SINGLE_FAILURE`, we add `AddFeeAndVsize()` or if we have a `TX_MISSING_INPUTS`, we `SkipWithDescendants()` the transaction. All oth
...
💬 hebasto commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1656668537)
> In the meantime it may be good to do 10 runs and then check how many of them fail
Observing intermittent "Error: no RPC connection". Converting this PR to a draft for now
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1656668537)
> In the meantime it may be good to do 10 runs and then check how many of them fail
Observing intermittent "Error: no RPC connection". Converting this PR to a draft for now
📝 hebasto converted_to_draft a pull request: "ci: Run Windows native task on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28173)
From https://github.com/bitcoin/bitcoin/issues/28098:
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.
Historical context:
- https://github.com/bitcoin/bitcoin/pull/17697
- https://github.com/bitcoin/bitcoin/issues/17803
- https://github.com/bitcoin/bitcoin/pull/18031
Security concerns:
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-16514321
...
(https://github.com/bitcoin/bitcoin/pull/28173)
From https://github.com/bitcoin/bitcoin/issues/28098:
> Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.
Historical context:
- https://github.com/bitcoin/bitcoin/pull/17697
- https://github.com/bitcoin/bitcoin/issues/17803
- https://github.com/bitcoin/bitcoin/pull/18031
Security concerns:
- https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-16514321
...
💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278285111)
Because future developers won't (easily) know where the number comes from.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1278285111)
Because future developers won't (easily) know where the number comes from.
💬 Sjors commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1278285397)
I think it's fine to mention these examples.
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1278285397)
I think it's fine to mention these examples.
💬 hsjoberg commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1656705552)
Concept ACK.
This is helpful for second later protocols, including Lightning.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1656705552)
Concept ACK.
This is helpful for second later protocols, including Lightning.
💬 Sjors commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656707196)
Concept ACK, but happy to wait for legal opinions. Hopefully they clarify the risks in two separate categories:
1. Content the AI obtained somewhere else without permission (i.e. claims from the original author)
2. Content the AI generated itself. Potentially owned by some corporation who didn't give permission to the person making the pull request to MIT license it.
When it comes to (1) I'm more worried about snippets of fresh code than e.g. suggested refactorings. I don't see how one
...
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656707196)
Concept ACK, but happy to wait for legal opinions. Hopefully they clarify the risks in two separate categories:
1. Content the AI obtained somewhere else without permission (i.e. claims from the original author)
2. Content the AI generated itself. Potentially owned by some corporation who didn't give permission to the person making the pull request to MIT license it.
When it comes to (1) I'm more worried about snippets of fresh code than e.g. suggested refactorings. I don't see how one
...