Bitcoin Core Github
44 subscribers
122K links
Download Telegram
👍 fanquake approved a pull request: "test: Remove windows workaround in authproxy"
(https://github.com/bitcoin/bitcoin/pull/27418)
ACK fa584b4d01ca055488a10b280f45dca323b474d4
👍 hebasto approved a pull request: "ci: use clang-16 in tidy task"
(https://github.com/bitcoin/bitcoin/pull/27404)
ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd
💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497308824)
I was able to reproduce this locally on master by running:

```
make distclean
FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
```

I ssh'd into the container and found that the `git diff` command is failing due to `dubious permissions`, which I think `./ci/lint/docker-entrypoint.sh` is supposed to fix with `git config --global --add safe.directory /bitcoin`? Seems like maybe that fix isn't working everywhere but we didn't catch it before because of the whole two use
...
💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497313723)
> `./ci/lint/` ...

The lint system is completely different from `./ci/test/`.
🚀 fanquake merged a pull request: "test: Remove windows workaround in authproxy"
(https://github.com/bitcoin/bitcoin/pull/27418)
💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497316092)
> The lint system is completely different from ./ci/test/.

yeah, just realized there is no overlap. seems like we just need to apply the same fix the lint system is using in `./ci/test/` ?
💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497316877)
As for the fix, anything should be fine. git `safe.directory`, or restoring the `chown`, or fiddling with the `rsync` command.
💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497318317)
> As for the fix, anything should be fine. git safe.directory, or restoring the chown, or fiddling with the rsync command.

cool, I'll open a PR
💬 Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497321029)
I agree with @Sjors here. In my last review comment, I suggested the same idea. Though I'm not sure how the peer privacy issue is relevant here. But, it makes more sense to only log ASN when <code>fLogIPs</code> is already true.

Maybe changing the name of <code>-logips</code> flag to something like <code>-lognetinfo</code> will better reflect its use and avoid confusion, but that would require a release note. (just a school of thought)
💬 MarcoFalke commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497326083)
lgtm ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd
🚀 fanquake merged a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
📝 josibake opened a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
fixes https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1496449588

this appears to be caused by a more recent version of git being sensitive to mismatched permissions on directories. we didn't notice this before because we were using two separate user accounts to fix up dir permissions in the container , but the second account was removed in #27376

there might be a more elegant way to do this, but this does the trick and seems to be the way others are fixing this issue around the
...
💬 josibake commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1497346650)
cc @hebasto @MarcoFalke
💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497347621)
Reading that original discussion I think the current code is fine. The main concern was exposing the origin of transactions in log files under the _default_ configuration. Since we don't log transaction relay by default (it's extremely noisy) we can safely log the ASN (or even IP) of a node when it connects.

We can clean up the documentation and/or behavior of `-logips` in another PR.
💬 josibake commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497365274)
ACK https://github.com/bitcoin/bitcoin/pull/27404/commits/a56c96507a9e943bbcd7e126bc827de9495f0ebd

> Included this. I think we could merge this as-is, but should also figure out why it's needed. It feels like there is another issue here.

did a little digging and found this: https://github.com/include-what-you-use/include-what-you-use/issues/679#issuecomment-482798379 , which seems to still be an issue based on the discussion in the linked solution. fwiw, the fix you have here seems to be o
...
👍 hebasto approved a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
ACK 2c11e94131447bc605b7698a80d02c2523497a4b, tested on Ubuntu 22.04.
💬 hebasto commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158438615)
A stray `#`?
💬 josibake commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#discussion_r1158448194)
fixed
💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1158456950)
yep thanks, fixed
👍 hebasto approved a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
re-ACK ed4a8339b8fe796b4668e206d7fb9c2b120f8eb2