💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1670965334)
I'd like to keep this based on #28122 but that requires some refactoring there. See https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1670943979.

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1670999533)
> I'd like to keep this based on #28122 but that requires some refactoring there. See [#28122 (comment)](https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1670943979).

you can also rebase on https://github.com/bitcoin/bitcoin/pull/27827 , which has everything, or just the receiving PR. Might be easier then to reason about which functions belong in which PRs if we can see it all together. I think the functions you need are introduced in the receiving PR (#28202)

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1288228607)
Removed mention of "modified," as it's not really relevant whether it's real fees or not

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288237266)
> That seems to be the case when opening this against my fork: [jarolrod#2](https://github.com/jarolrod/bitcoin/pull/2)

That's correct. But is this really the case we want to care about?

👍 dergoegge approved a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240#pullrequestreview-1569180518)
utACK faaba770e11352ddf6414b9855f4baa46a967580

📝 vincenzopalazzo opened a pull request: "Revert "Remove unused raw-pointer read helper from univalue""
(https://github.com/bitcoin/bitcoin/pull/28242)
This reverts commit fa940f41eaffa4b2a28c465a10a4c12d4b8976b8.

With this commit produce the following compilation error.

```
Making all in src
make[1]: Entering directory '/home/vincent/Github/bitcoin/src'
make[2]: Entering directory '/home/vincent/Github/bitcoin/src'
make[3]: Entering directory '/home/vincent/Github/bitcoin'
make[3]: Leaving directory '/home/vincent/Github/bitcoin'
GEN obj/build.h
CXX libbitcoin_util_a-clientversion.o
AR libbitcoin_util.a
C
...

💬 dergoegge commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671025800)
I had this too and iirc `make clean && make` worked for me.

💬 vincenzopalazzo commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671028960)
>I had this too and iirc make clean && make worked for me.

Mh, let me try

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1671034496)
Updated fe34609476b11cdd907c298f7300d424bf556e98 -> 9658d0dc17c270138523c41a982425e276b24271 ([pr28187.05](https://github.com/hebasto/bitcoin/commits/pr28187.05) -> [pr28187.06](https://github.com/hebasto/bitcoin/commits/pr28187.06), [diff](https://github.com/hebasto/bitcoin/compare/pr28187.05..pr28187.06)):

- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288122926)
- the workflow run name is set as the [commit message](https://docs.github.com/e
...

👍 fanquake approved a pull request: "ci: Use qemu-user through container engine"
(https://github.com/bitcoin/bitcoin/pull/28087#pullrequestreview-1569205742)
ACK fad0b67c212dcb8a16fcbda5a74acc959ed4e284 - this seems ok to me, and removes complexity from our CI system.

I've locally tested the ENVs changed here, and seen the associated slowdown / resource consumption. Had GCC segfault more than once when cross-compiling s390x.

🚀 fanquake merged a pull request: "ci: Use qemu-user through container engine"
(https://github.com/bitcoin/bitcoin/pull/28087)

💬 vincenzopalazzo commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671049248)
It worked, thanks! sorry for the confusion here

✅ vincenzopalazzo closed a pull request: "Revert "Remove unused raw-pointer read helper from univalue""
(https://github.com/bitcoin/bitcoin/pull/28242)

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288266548)
> It is not possible to "move" a const reference into a shared_ptr. It will be a copy again.

Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copie
...

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288269938)
> Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copies?

Pretty sure it doesn't, when `get_type()` returns a `const std::string&`, both `auto&` an
...

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288270672)
> But if `get_type()` returns by value, how would we use a raw pointer? Who owns the underlying? That's why I went for `shared_ptr`, but I'm still easily confused by memory management so apologies if I'm being dim.

`std::optional` can be used to own optional memory, see my previous reply to the thread and the current version of the code :)

👍 hebasto approved a pull request: "ci: Move tidy and macOS-cross to persistent workers"
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1569225408)
ACK fa960fefb589ac97de206f46d7b5c565fc70e3db. I have reviewed the code and it looks OK.

However, I'm a bit reluctant about running macOS-cross job on a persistent worker. I'd prefer run it on GitHub Actions due to the ability to create downloadable artifacts that is used in the QML repo to make designers' life a bit easier.

I understand, that there is no open PR with such a suggestion for now. But there is an ongoing Linux jobs related discussion in https://github.com/bitcoin-core/secp256k
...

💬 MarcoFalke commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277494)
Good catch. The test was written by @vasild. I've removed the code, since there is already another unit test checking `SERIALIZE_METHODS_PARAMS`.

Also, I've replaced `DEC` by `RAW` to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core's own util functions.

💬 MarcoFalke commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277658)
Thanks, fixed doxygen

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1671071478)
> Had GCC segfault more than once when cross-compiling s390x.

I think this can be fixed by setting `MAKEJOBS="-j1" FILE_ENV=..`. (or a higher value, depending on your memory size)