💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279351539)
nit: Can drop the comment? (The others don't have a comment either)
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279351539)
nit: Can drop the comment? (The others don't have a comment either)
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279321024)
This commit is neither a refactor, nor is it correct. `\n` is already included in `base`.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279321024)
This commit is neither a refactor, nor is it correct. `\n` is already included in `base`.
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279330906)
Could add a reason why this should be avoided?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279330906)
Could add a reason why this should be avoided?
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069)
I think the test isn't run in CI, is it?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069)
I think the test isn't run in CI, is it?
👍 MarcoFalke approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1554817965)
ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b 🗑
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 3a727cd7ee02c83efcd57d004e
...
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1554817965)
ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b 🗑
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 3a727cd7ee02c83efcd57d004e
...
💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1658468351)
Concept ACK
I still suggest changing fc810c1df899e5b42fe7658ebdddc6b4320240a6 so that `dumpwallet` does not use the read-only format by default, and instead change the tests use it. However with the new fuzz target commit I'm less worried about it.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1658468351)
Concept ACK
I still suggest changing fc810c1df899e5b42fe7658ebdddc6b4320240a6 so that `dumpwallet` does not use the read-only format by default, and instead change the tests use it. However with the new fuzz target commit I'm less worried about it.
💬 BitcoinMechanic commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658548723)
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC we have seen circa 350k unique trx hash per month (over the last 3 months) requested to our platform. Our clients include - Coinpayment
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658548723)
> This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
>
> At GAP600 we continue to see strong use of our service for BTC we have seen circa 350k unique trx hash per month (over the last 3 months) requested to our platform. Our clients include - Coinpayment
...
💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279447033)
Maybe add a recommendation to the fuzz doc to [mount /tmp to tmpfs](https://ubuntu.com/blog/data-driven-analysis-tmp-on-tmpfs), or ensure we don't use the file system.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279447033)
Maybe add a recommendation to the fuzz doc to [mount /tmp to tmpfs](https://ubuntu.com/blog/data-driven-analysis-tmp-on-tmpfs), or ensure we don't use the file system.
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279450655)
This is already in the test docs, no?
```
$ git grep tmpfs test/README.md
test/README.md:sudo mount -t tmpfs -o size=4g tmpfs /mnt/tmp/
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279450655)
This is already in the test docs, no?
```
$ git grep tmpfs test/README.md
test/README.md:sudo mount -t tmpfs -o size=4g tmpfs /mnt/tmp/
💬 jonathanbier commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658569312)
When I first read this, I did not understand what those links to https://mempool.space/ were, as I did not know, but they seem to have a new RBF feature. This RBF information then seems to go after a few days. Therefore I have provided some screenshots below so people can see the feature and it wont disappear. It shows that the transactions were replaced and that there was no RBF flag. I hope this helps.
[Antpool](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658569312)
When I first read this, I did not understand what those links to https://mempool.space/ were, as I did not know, but they seem to have a new RBF feature. This RBF information then seems to go after a few days. Therefore I have provided some screenshots below so people can see the feature and it wont disappear. It shows that the transactions were replaced and that there was no RBF flag. I hope this helps.
[Antpool](https://mempool.space/tx/53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122
...
💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279466130)
It would be useful here - if BDB is compiled - to also open it with BDB, dump and then compare both dumps.
I'm not sure what to do for cases where BDB and RO have slightly different opinions on what they're willing to open at all. It's probably fine to skip comparing files where _either_ refuses to open.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279466130)
It would be useful here - if BDB is compiled - to also open it with BDB, dump and then compare both dumps.
I'm not sure what to do for cases where BDB and RO have slightly different opinions on what they're willing to open at all. It's probably fine to skip comparing files where _either_ refuses to open.
💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279468544)
Not in the fuzz docs, but just having it in the test docs is probably fine.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1279468544)
Not in the fuzz docs, but just having it in the test docs is probably fine.
📝 theStack opened a pull request: "test: add script compression coverage for not-on-curve P2PK outputs"
(https://github.com/bitcoin/bitcoin/pull/28193)
This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](https://github.com/bitcoin/bitcoin/blob/44b05bf3fef2468783dcebf651654fdd30717e7e/src/pubkey.cpp#L297-L302), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't
...
(https://github.com/bitcoin/bitcoin/pull/28193)
This PR adds unit test coverage for the script compression functions `{Compress,Decompress}Script` in the special case of uncompressed P2PK outputs (scriptPubKey: OP_PUSH65 <0x04 ....> OP_CHECKSIG) with [pubkeys that are not fully valid](https://github.com/bitcoin/bitcoin/blob/44b05bf3fef2468783dcebf651654fdd30717e7e/src/pubkey.cpp#L297-L302), i.e. where the encoded point is not on the secp256k1 curve. For those outputs, script compression is not possible, as the y coordinate of the pubkey can't
...
💬 fanquake commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1279500464)
Maybe relevant here, I think it's easier to just avoid (explicitly) copy-pasting ChatGPT or similar output into the project, for now, see #28175.
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1279500464)
Maybe relevant here, I think it's easier to just avoid (explicitly) copy-pasting ChatGPT or similar output into the project, for now, see #28175.
💬 achow101 commented on pull request "ParseHDKeypath: support h as hardened marker":
(https://github.com/bitcoin/bitcoin/pull/28192#issuecomment-1658730282)
I'm not sure if there's any software that enforces the consistent usage of a hardened marker.
(https://github.com/bitcoin/bitcoin/pull/28192#issuecomment-1658730282)
I'm not sure if there's any software that enforces the consistent usage of a hardened marker.
💬 ryanofsky commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1658798341)
NACK from me, because I think legal questions like this are essentially political questions, and you make absurd legal outcomes more likely to happen by expecting them to happen, and by writing official documentation which gives them credence.
If the risk is that openai or microsoft could claim copyright over parts of the bitcoin codebase, that would be absurd because their usage agreements assign away their rights to the output and say it can be used for any purpose.
If the risk is that s
...
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1658798341)
NACK from me, because I think legal questions like this are essentially political questions, and you make absurd legal outcomes more likely to happen by expecting them to happen, and by writing official documentation which gives them credence.
If the risk is that openai or microsoft could claim copyright over parts of the bitcoin codebase, that would be absurd because their usage agreements assign away their rights to the output and say it can be used for any purpose.
If the risk is that s
...
💬 kristapsk commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658851397)
> 2\. Users who do care and want full-RBF to be **off** and rely on defaults not changing.
Users should check release notes before upgrading Bitcoin node. Various defaults changes from time to time, this won't be the first time.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658851397)
> 2\. Users who do care and want full-RBF to be **off** and rely on defaults not changing.
Users should check release notes before upgrading Bitcoin node. Various defaults changes from time to time, this won't be the first time.
📝 jonatack opened a pull request: "test, script: python E721 and flake8 updates"
(https://github.com/bitcoin/bitcoin/pull/28194)
Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green.
```
$ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py
test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance c
...
(https://github.com/bitcoin/bitcoin/pull/28194)
Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green.
```
$ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py
test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance c
...
💬 luke-jr commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1658950229)
>...the purpose of including signet in this repo is about making it easier to test bitcoin, not about building your own altcoin with different parameters. How does using a 30s signet do anything but encourage developers to build software that will only work well on altcoins (like liquid...
Just thought I should point out that Liquid is Bitcoin, not an altcoin.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1658950229)
>...the purpose of including signet in this repo is about making it easier to test bitcoin, not about building your own altcoin with different parameters. How does using a 30s signet do anything but encourage developers to build software that will only work well on altcoins (like liquid...
Just thought I should point out that Liquid is Bitcoin, not an altcoin.
💬 jonatack commented on pull request "test, script: python E721 and flake8 updates":
(https://github.com/bitcoin/bitcoin/pull/28194#issuecomment-1658963090)
CI lint task report looks good https://cirrus-ci.com/task/6522164438892544.
(https://github.com/bitcoin/bitcoin/pull/28194#issuecomment-1658963090)
CI lint task report looks good https://cirrus-ci.com/task/6522164438892544.