💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282837030)
Ah, actually I didn't think about temporaries at all... I changed it to be more in line with `Serialize` which uses `const Args&... args`, so `Args&... args` felt more appropriate.

I'll change it back to &&

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1282854035)
This issue has some helpful context: https://github.com/bitcoin/bitcoin/issues/12314#issuecomment-582600899

TLDR; at the time regtest was created, it was assumed that using a separate HRP wouldn't cost anything.

> If each of the four had a different one, I’d get it, but why does regtest have a different one than the other two testing networks?

This actually makes sense to me. Testnet and Signet are "global" networks, so everyone has to agree on the address format. Regtest is always used
...

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1663552242)
@hebasto did you open an issue to track this upstream? Can you link it here?

💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#issuecomment-1663556136)
Rebased to f054bd0 to clang-format the changes, and adds back the `&&` in `UnserializeMany`.

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1663560293)
> There's still another crash that I have

Not sure if that was fixed in your later push, but in any case, I pushed another crash to the qa-assets PR.

🤔 MarcoFalke reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1560573265)
(nits only)

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282873918)
nit in 167cbf7b1851eeeb28937bd8e241e1f865b813f8:


```cpp
int64_t r{std::ftell(m_file)};

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282875416)
nit in 167cbf7b1851eeeb28937bd8e241e1f865b813f8:

Could add `std::` before `fseek`? Also, the commit message has a typo:

usefule -> useful (remove `e`)

⚠️ feed3r opened an issue: "Master doesn't compile on MacOS X 10.13 High Sierra"
(https://github.com/bitcoin/bitcoin/issues/28206)
Hello guys, I'm trying to compile the current Master branch on a quite old Macbook Pro (early 2011) for which the latest supported os is Mac OS X 10.13 high sierra, and I'm getting the following error:

```
Alessios-MacBook-Pro:src feeder$ make V=1
g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -Xclang -internal-isystem/usr/local/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/inc
...

💬 Sjors commented on pull request "net_processing: re-allow fetching of genesis block via `getblockfrompeer`":
(https://github.com/bitcoin/bitcoin/pull/28205#issuecomment-1663573528)
Or even simpler: have `getblockfrompeer` fail immediately if you ask it for the genesis block.

💬 fanquake commented on issue "Master doesn't compile on MacOS X 10.13 High Sierra":
(https://github.com/bitcoin/bitcoin/issues/28206#issuecomment-1663577646)
> that does not fully support C++17

Master requires a C++17 capable compiler, so you wont be able to compile it with a compiler that doesn't support C++17. I'd suggest installing a modern LLVM/Clang via brew, and using that, if possible.

> Does anybody know if MacOS 10.13 is not supported anymore
> Is it known if MacOS 10.15 Catalina is supported?

On master, the minimum supported macOS version is 11.0. The prior minimum supported macOS version was 10.15.

🚀 fanquake merged a pull request: "qa: Close SQLite connection properly"
(https://github.com/bitcoin/bitcoin/pull/28204)

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1282885877)
Addressed this.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1663582461)
Moving back to draft while we follow up with more suggestions.

📝 fanquake converted_to_draft a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)
Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job.

The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter.

💬 Sjors commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#issuecomment-1663589369)
Maybe add a quick summary in the description with the main implementation differences relative to #24897. It seems a big one is that this doesn't require an index!

📝 MarcoFalke opened a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207)
Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart.

Fix this, similar to https://github.com/b
...

💬 Sjors commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#issuecomment-1663592220)
Maybe add a quick summary in the description with the main implementation differences relative to #24897. It seems a big one is that this doesn't require an index!

You should also link from the child PRs back to this one.

💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1663593182)
Currently opened as draft to wait for initial feedback. There is no option to disable this feature, because I am not aware of anyone reading the `mempool.dat`, is there? (The `getrawmempool` RPC is the recommended way to get the mempool, and using the `savemempool` RPC instead seems like an edge-case?)

📝 fanquake opened a pull request: "lint: remove pkg_resources usage"
(https://github.com/bitcoin/bitcoin/pull/28208)
`pkg_resources` is deprecated, and warns with newer Python:
```bash
/bitcoin/test/lint/lint-python.py:12: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
import pkg_resources
```

Switch to using `importlib.metadata`, which has existed since Python 3.8.

See: https://docs.python.org/3/library/importlib.metadata.html#module-importlib.metadata.
See: https://setuptools.pypa.io/en/latest/pkg_resources.html