Bitcoin Core Github
43 subscribers
123K links
Download Telegram
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217480535)
The comment gives the impression that `sa_family_t` is defined on Windows (as `u_short`), but it is not defined at all (otherwise this `typedef u_short sa_family_t;` would not compile because it redefines it, right?). Maybe reword the comment to something like:

```cpp
// Windows does not have `sa_family_t` - it defines `sockaddr::sa_family` as `u_short`.
// Thus define `sa_family_t` on Windows too so that the rest of the code can use `sa_family_t`.
```
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217719465)
All tuning that happens to the socket below in this function seems relevant to UNIX sockets too, except `TCP_NODELAY`.

```diff
--- i/src/netbase.cpp
+++ w/src/netbase.cpp
@@ -484,23 +484,21 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a

std::unique_ptr<Sock> CreateSockOS(const sa_family_t& address_family)
{
// Not IPv4, IPv6 or UNIX
if (address_family == AF_UNSPEC) return nullptr;

+ int protocol{IPPROTO_TCP};
#if HAVE_SOCKADD
...
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217741453)
Pass `path` by const reference, strings are expensive to copy.

```suggestion
explicit Proxy(const std::string& path, bool _randomize_credentials=false): unix_socket_path(path), is_unix_socket(true), randomize_credentials(_randomize_credentials) {}
```
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217743952)
Members should start with `m_`. Maybe also rename the existent `randomize_credentials` to `m_randomize_credentials` (in a separate commit).
🚀 fanquake merged a pull request: "wallet: Add tracing for sqlite statements"
(https://github.com/bitcoin/bitcoin/pull/27801)
💬 MarcoFalke commented on pull request "rpc: add getxpub":
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1576494307)
Maybe mark as draft if this depends on something?
💬 MarcoFalke commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1576501170)
Needs rebase?
💬 MarcoFalke commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1576502477)
Needs rebase?
📝 Sjors converted_to_draft a pull request: "rpc: add getxpub"
(https://github.com/bitcoin/bitcoin/pull/22341)
Depends on #26728

This PR introduces a new wallet RPC `getxpub`. It takes a BIP32 path as argument and returns the xpub, along with the master key fingerprint.

To test this PR:

* create a regular descriptor wallet
* call `getxpub m/86h/0h/0h`
* call `listdescriptors`, compare the xpub in the `tr()` descriptor with the previous step

## Bigger picture

This paves the way for using Bitcoin Core as one signer in a multisig setup. It simplifies the proposed flow in #22067.

The even
...
💬 Sjors commented on issue "Log which peer sent us a header (first)":
(https://github.com/bitcoin/bitcoin/issues/27744#issuecomment-1576530976)
@Harshil-Jani no need to ask permission. That said, validation and net_processing are usually not the best place to start if you're new to this code. It's easy to make mistakes here, even with something as simple as adding a log entry (e.g. #27673).
💬 kristapsk commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#issuecomment-1576530999)
> Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.

It's pros and cons. Repo will have more signatures, but any centralized server is single point of failure, torrents are better in this regard.
💬 willcl-ark commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#issuecomment-1576542975)
Sure there are tradeoffs.

It also seems problematic in that fetching them in any sane way would require `git` to be installed on the host machine (so we could e.g. do a sparse checkout of the release sub-directory). Alternatively just download the entire repo as .zip and go from there? Both seem undesirable/"messy" to me personally. 🤷🏼‍♂️
💬 Harshil-Jani commented on issue "Log which peer sent us a header (first)":
(https://github.com/bitcoin/bitcoin/issues/27744#issuecomment-1576553817)
I see. Thanks for this, I will try to look towards something else.
📝 fanquake opened a pull request: "ci: enable AArch64 target in MSAN jobs "
(https://github.com/bitcoin/bitcoin/pull/27824)
Make it possible to run the MSAN jobs on aarch64, as it was previously.
💬 MarcoFalke commented on pull request "ci: enable AArch64 target in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1217916568)
will this build both? Would be nice to just detect and build for the current CPU, if that is possible?
⚠️ vasild opened an issue: "Use semantic analysis in lint-logs.py"
(https://github.com/bitcoin/bitcoin/issues/27825)
### Please describe the feature you'd like to see added.

Currently `test/lint/lint-logs.py` tries to parse the C++ manually which is prone to flaws. For example it does not support expressions that span more than one line and would be happy if there is `\n"` anywhere (on the single line it checks). For example:

```cpp
LogPrintLevel(BCLog::PROXY,
BCLog::Level::Debug,
"This is fine, but lint-logs.py will report error\n");
```

```cpp
...
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1576619633)
Updates:
- It seems I broke some functional tests with the `WriteReply` function "fix", ha, working on it.
💬 MarcoFalke commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1217942753)
Looks like this doesn't work some of the time:

* https://cirrus-ci.com/task/5798762795237376?logs=ci#L267
* https://cirrus-ci.com/task/4640353173635072?logs=ci#L266

```
++ echo 'Restart docker before run to stop and clear all containers started with --rm'
Restart docker before run to stop and clear all containers started with --rm
++ podman container kill --all
++ echo 'Prune all dangling images'
Prune all dangling images
++ docker image prune --force
Emulate Docker CLI using podma
...
📝 Sjors opened a pull request: "validation: log which peer sent us a header"
(https://github.com/bitcoin/bitcoin/pull/27826)
Fixes #27744

Since #27278 we log received headers. For compact blocks we also log which peer sent it (e5ce8576349d404c466b2f4cab1ca7bf920904b2), but not for regular headers. That required an additional refactor, which this PR provides.

Move the logging from validation to net_processing.

This also reduces the number of log entries (under default configuration) per compact block header from 3 to 2: one for the header and one for the connected tip.

The PR introduces a new helper method
...
💬 hebasto commented on pull request "ci: enable AArch64 target in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1217967152)
> Would be nice to just detect and build for the current CPU, if that is possible?

What about not providing the value at all, and using the default one?