💬 kevkevinpal commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1654947763)
ACK [08f9f62](https://github.com/bitcoin/bitcoin/pull/28175/commits/08f9f62dc4c1f2b2864d9a5f78b4f490b0debe87)

I was one of the mentioned PR's https://github.com/bitcoin/bitcoin/pull/28101

Would make sense to have this in the CONTRIBUTING.md

🤔 ariard reviewed a pull request: "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)"
(https://github.com/bitcoin/bitcoin/pull/28175#pullrequestreview-1551270145)
To be honest, and after looking on some LLM-term of service and in basic knowledge of copyright law, there is an uncertainty on the status of LLM output. It sounds LLM or AI operating platforms in their terms of service do no make the claim they own the intellectual property of the LLM output, and if even if they do so it’s probably an unfounded claim. A user might mix an “original” or “creative” element by sending an individual prompt request, a determining factor in any matter of intellectual
...

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1277040875)
I would recommend to drop any reference to a corporate entity, or one of its product in Bitcoin Core documentation, to avoid a mischaracterization of what they’re doing (whatever one personal opinion).

We already mentioned Github a lot in the contributing.md, though only as the technical platform where contributions are happening, not taking a stance on one of their product.

📝 kevkevinpal opened a pull request: "tests: add coverage to feature_addrman.py"
(https://github.com/bitcoin/bitcoin/pull/28176)
I added two new tests that will cover the nNew and nTried tests which add coverage to the if block by checking values larger than our range since we only check for negative values now

adding coverage to these lines
https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L273
https://github.com/bitcoin/bitcoin/blob/master/src/addrman.cpp#L280

our test seem to only cover the `nTried < 0` and `nNew < 0` scenarios
<!--
*** Please remove the following help text before submitting: ***
...

💬 MarcoFalke commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1655060132)
Can you remove the Windows Cirrus task here, as it will need to be removed either way? Also, a link to a build would be nice, before this is enabled here.

💬 MarcoFalke commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#issuecomment-1655061310)
Maybe also print the `GITHUB_TOKEN` permissions at every start of the task, just to double check they are read-only?

💬 MarcoFalke commented on pull request "ci: Run Windows native task on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28173#discussion_r1277118408)
```suggestion
on: [pull_request, push] # https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore
```

👍 MarcoFalke approved a pull request: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153#pullrequestreview-1551387955)
lgtm

💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1277122561)
nit: Could use `PickValue`?

🤔 MarcoFalke requested changes to a pull request: "rpc, util: avoid string copies in ParseHash/ParseHex functions"
(https://github.com/bitcoin/bitcoin/pull/28172#pullrequestreview-1551412773)
Not sure. This has no effect on the compiled binary and is only changing the style of the code.

💬 MarcoFalke commented on pull request "rpc, util: avoid string copies in ParseHash/ParseHex functions":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1277134227)
The names are short, so a copy or reference should make no difference. Also, a copy is still done when the input is a c-string, which is the case here for all call sites.

🤔 MarcoFalke requested changes to a pull request: "test: blockstore test with chattr instead of chmod"
(https://github.com/bitcoin/bitcoin/pull/28171#pullrequestreview-1551429665)
I think you can just use the existing pull request to push the changes. It is clear that this approach will be working. Just pay attention to the details and make sure you are confident in your diff before you push, if possible.

✅ MarcoFalke closed a pull request: "test: blockstore test with chattr instead of chmod"
(https://github.com/bitcoin/bitcoin/pull/28171)

💬 MarcoFalke commented on pull request "test: blockstore test with chattr instead of chmod":
(https://github.com/bitcoin/bitcoin/pull/28171#discussion_r1277145643)
This has nothing to do with asan, you will have to add it to all created containers.

💬 MarcoFalke commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1277155809)
Not sure. There were previously two functions and there still are two functions now, with more code at the call sites. So I am not sure if this is an improvement. The only difference seems to be in the throw function, so in theory you could pass in a lambda that takes care of the throw, but I am not sure if this is worth it.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277157308)
> Pushed a commit here which tests this: [theuni/bitcoin-tidy-plugin@cdc0070](https://github.com/theuni/bitcoin-tidy-plugin/commit/cdc007022b6cd40320b7e74ab4cbb8e67a6d6e17)
>
> It works as expected.

Nice. Maybe include this here?

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1277194218)
I have changed this to read ```Invalid or unsupported prefix```... testing my own knowledge; The reason you can have an unsupported but still technically valid address would be in the event of a softfork to a new SegWit version? e.x. _a segwit client interpreting a taproot address would say "invalid"_ but that is not completely true because the address is valid. Just not supported by the client?

This has been resolved in b66e974ac8f21971602051c08df830d14cf3df7e

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1277247979)
Just to leave my thought (which I withheld prior to merge): Could just use the raw (single) byte here, but that would interfere with libFuzzer `-only_ascii=1`, which makes me wonder if this is the first ascii fuzz target and whether we should set the option somewhere somehow during input generation?

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1277267221)
Thank you, I updated with your suggestions

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1277267649)
Done, thanks