💬 furszy commented on issue "Unsafe reduce_output when new coins are added":
(https://github.com/bitcoin/bitcoin/issues/28180#issuecomment-1659172967)
> Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

I don't think so. This is more an implementation issue rather than a missing doc or a missing restriction.
This could also happen without someone intending to do it. For instance, could be a higher bump fee that make the tx require another input.

The issue arises because the code (as is now) mixes two different concepts; it treats the change outpu
...

🤔 jonatack reviewed a pull request: "script: throw disabled err for op_ver and its variants"
(https://github.com/bitcoin/bitcoin/pull/28169#pullrequestreview-1555801363)
Approach ACK ffc32938aa962f7c4b8d5f4af69bd710c174ef44

The unit script tests (`./src/test/test_bitcoin -t script_tests`) don't pass at the first commit, so it may be best to combine the second commit updating the tests into the first commit with the code changes.

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659270516)
squashed into single commit ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

👍 theStack approved a pull request: "test: Drop 22.x node from TxindexCompatibilityTest"
(https://github.com/bitcoin/bitcoin/pull/28070#pullrequestreview-1555894628)
Code-review ACK fafe43cb6c76a5f60194be128a40baf161d39920

Additional replacement suggestion for the scripted-diff (last commit), to also tackle chain_path + "blocks" concatenations that are done via os.path.join:
`sed -i 's|].chain_path, "blocks"|].blocks_path|g' $(git grep -l chain_path)`

💬 jonatack commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659278229)
ACK ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

- It looks like the first sentence of the PR description needs to be updated.
- Would it be good to accompany this change with a release note?

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#issuecomment-1659280674)
Rebased after #27888.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1659282959)
re-ACK 8f6eeaab38

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659290755)
updated PR description

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1279979090)
I think a) there is no certainty ChatGPT / LLaMa will be the most popular framework 12 / 18 months from now and I don’t think we’re going to update contributing rules everytime and b) Meta is a registered trademark of a commercial entity and I think it’s better to not give the appearance Bitcoin Core the project is supportive or supported or linked to Meta in anyway.

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1659356787)
I had the feedback from the Bitcoin Defense Legal Fund, they need more time to analyze the issue though it is thought as an important one.

I did additionally cc Andrew Chow and Fanquake as maintainers on the mail thread for open-source transparency.

I think whatever one individual political opinion on copyrights or legal risks tolerance, the fact is we have already dubious copyrights litigations affecting the project, so I think it’s reasonable to wait for risk clarification before to do a
...

💬 MarcoFalke commented on pull request "test, script: python E721 and flake8 updates":
(https://github.com/bitcoin/bitcoin/pull/28194#issuecomment-1659613454)
lgtm ACK bee2d57a655645dbfaf0242e85c5af034023a2fb

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659618783)
Approach NACK, (but Concept ACK). Leaving a NACK only to have DrahtBot create an anchor to this comment, to avoid GitHub from hiding it as well.

I don't think you've seen https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387 and GitHub is already hiding it by default.

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279295208)
```suggestion
// Ensure sensitive relay connections only send the above message types. Others are not needed and may degrade privacy.
```

(nit), to avoid having to touch this line again in the future, if something changes.

💬 glozow commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1280249089)
I think I know what you mean, but imo transactions being stuck is a little bit of a jump from feerate underestimation. Maybe "Serving stale fee estimates may lead to overpaying in fees or not meeting minimum feerate requirements in the worst case." Or just delete this comment, as the code is self-explanatory and there is already a comment above `MAX_FILE_AGE`.

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1280251267)
It's related to https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277639934

💬 Sjors commented on pull request "ParseHDKeypath: support h as hardened marker":
(https://github.com/bitcoin/bitcoin/pull/28192#issuecomment-1659783573)
We could always relax the constraint if someone has a good use case for not being consistent. For now this is method is only foreseen to be used with `getxpub` where I can't think of an even remotely sane reason. More likely someone will do this by accident and end up confused why their descriptor checksum doesn't match.

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1659784274)
Thanks, makes sense. I initially wondered about the 15 commits and +100 LOC, but having more commits for this template-heavy code makes it easier to review, and I found a way to reduce the number of LOC. Just a style nit though, feel free to ignore.

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280179325)
unrelated nit in 4735590b9f3bdfc248086e0e20c9d56fa6156e1c: Add missing `}; // namespace dbwrapper_private` (via clang-format) to clarify the new function is not in the namespace?

👍 MarcoFalke approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556273554)
review ACK 0280dc44d227ae34e8fcbb708833bfe9d66c7b5f 🔨

<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: review ACK 0280dc44d227
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280188384)
74b5c3dac581b7597668a6a0cc55678d2af296da: Any reason to remove the docs from the header?