💬 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
...

💬 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.

📝 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
...

💬 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.

💬 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.

👋 jonatack's pull request is ready for review: "test, script: python E721 and flake8 updates"
(https://github.com/bitcoin/bitcoin/pull/28194)

🤔 pinheadmz reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1555409244)
code review re-ACK 2541f09439099ec3e73f7c5a12f809f190e6af1d

confirmed changes since last ack were rebase and small review comments addressed. Found a typo, and played with the feature on signet (longer report about that to follow)

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279655081)
nit: s/send/sent

💬 willcl-ark commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1279739808)
nit: should we `sys.exit(1)` here too?

👍 willcl-ark approved a pull request: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-1376557345)
tACK 3ce180ac2d

Left two nits which don't need addressing unless being re-touched, but overall this works well in testing and seems like a useful contrib script. Converting the output to json also worked as described in the comments above.

💬 willcl-ark commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1160947835)
Whitepsace nit (unless it was deliberate to align with following line):

```suggestion
parser.add_argument('infile', help='filename of compact-serialized UTXO set (input)')
```

🤔 stickies-v reviewed a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1555593455)
Approach ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b

Did a code review too but both the build stuff as well as the clang-tidy syntax is still very new to me so doesn't mean too much.

Are the tests being run in CI with this PR? And perhaps some tidy-test for multiline strings would be helpful too?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659008138)
Ran with this feature on signet and found a few unexpected behaviors:

1. `-sensitiverelay=1` and `-onlynet=ipv4` should probably throw an init error. In this configuration, sensitive stuff seems to be ignored and new wallet TXs are relayed via ipv4 peers like normal. Is "only use tor for sensitive relay" a possible configuration?

2. Rapidly sending multiple transactions `"...incremented the number of connections to open from 20 to 25..."` this number grows rapidly and decreases slowly (esp
...

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279730123)
In 29d9d326d5193bb9a410a8881eabc93de5dd6266 "[txorphanage] track size of stored orphans, total and by peer"

This function is unimplemented.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279714521)
In 543273d96e896adf5531ed961856aa0eb70cbe57 "[log] log ProcessOrphanTx() events by wtxid"

Perhaps log both txid and wtxid?

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279743325)
In 974d864419dd98be6e32dec3ee11f5082b060b1b "[refactor] make TxPackageTracker responsible for EraseTx and AddChildrenToWorkset"

This line seems a bit unrelated to this commit as there is no pre-existing `EraseTx`.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279747416)
In b5ab45e595bbcedbd602b6385b83e9ffd983f216 "[p2p/refactor] make TxPackageTracker responsible for orphan resolution"

nit: indentation

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1555524219)
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279760417)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b7ce551cd403f8e2a099372915f6022ad4)

Would be good to directly verify the snapshot block is included:

```c++
// block before the snapshot is excluded (as well as earlier blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base->pprev), 0);
// snapshot block is included (as well as later blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base), 1);
``
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279811734)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277557586

> [0ce805b](https://github.com/bitcoin/bitcoin/commit/0ce805b632dcb98944a931f758f76f530f5ce5f2): why isn't `BLOCK_VALID_TRANSACTIONS` also alternatively `ASSUMED_VALID`?

I'm not sure what this review comment is asking. The code comment above still seems accurate to me, but it maybe it could be improved.