💬 Macoophonic commented on pull request "wallet: clarify replace fields in gettransaction":
(https://github.com/bitcoin/bitcoin/pull/27782#issuecomment-1570963364)
Wallet code It's so dark, thank you
Được gửi từ Yahoo Mail trên Android

Vào Th 5, 1 thg 6, 2023 lúc 4:00, Bruno ***@***.***> đã viết:


@brunoerg commented on this pull request.

Concept ACK

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

👍 ryanofsky approved a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1454166240)
Code review ACK 13476fe7bebdbf51e09821850b2c808c8ecf116a

💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (13476fe7bebdbf51e09821850b2c808c8ecf116a)

Maybe add a comment above the batch.Write() call like "Convert the key and value to DataStream objects in order to bypass serialization. We want raw bytes to be written to the database, not serialized byte strings. The DatabaseBatch::Write template method normally serializes its arguments, but because DataStream has a Serialize method that does concatenation instead of serialization, it can be
...

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570974530)
> I wasn't sure if you were going to add the named-only parameters to the conversion table

Sorry, I just missed the part of your earlier comment where you suggested to this. I didn't realize the conversion table could handle parameters not passed by position. Should be fixed now though.

Updated 2808c33bae95a0d2516a6e9a550032af142a04de -> 5559ad2c69ef82c18843b9214e5ba3974834a1ae ([`pr/nonly.15`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.15) -> [`pr/nonly.16`](https://github.com/
...

💬 LarryRuane commented on pull request "util: generalize accounting of system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1212354487)
Thanks, I got a slightly different result -- `malloc_usable_size() + sizeof(void*)` is perfect (note adding only `sizeof(void*)` instead of 2 times that). I'm not set up for 32-bit, can you try running this test program on 32-bit if you are able to easily?

Whichever is the most accurate, would a PR to improve [`memusage.h: MallocUsage()`](https://github.com/bitcoin/bitcoin/blob/6cf47a8f44f96099a84e5c6e628e3499045e024d/src/memusage.h#L50) be worth doing? (I would say a separate PR would be bet
...

📝 theuni opened a pull request: "depends: modernize clang flags for Darwin"
(https://github.com/bitcoin/bitcoin/pull/27798)
This is a cleaner and simpler alternative to #25098. Inspired by [this conversation](https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562543301). The diff is large but the change itself is quite small.

Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, this is much cleaner and more maintainable than what we had before.

See the updated comment for more info. At a high level: rather than playing tricks and trying to work around clang's default includes
...

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1571040493)
> > just need to decide out what to do about guix.
>
> What options are currently being considered. Is an upstream patch still possible?

See #27798 for an alternative that is imo better in every way :)

💬 mzumsande commented on pull request "p2p, rpc, test: `getpeerinfo` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1212373107)
The title and commit messages don't fit: This PR changes concerns `addnode`, not `getpeerinfo`.

💬 hebasto commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#issuecomment-1571046189)
Concept ACK.

💬 brunoerg commented on pull request "p2p, rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1212420718)
Ooops, I think things changed during the way, thanks for notice it! Done!

💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1571334843)
It looks like the conversion table is missing the arguments that take amounts; these also need to be converted. The test is also failing as the named only arguments are not being output in the `dump_all_command_conversions`, the regex can't handle negatives, and `psbt` has mismatched conversions.

The following diff should fix those issues:

```diff
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index be4d777169..7bd66ebbd2 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp

...

💬 torkelrogstad commented on pull request "wallet: clarify replace fields in help output":
(https://github.com/bitcoin/bitcoin/pull/27782#issuecomment-1571408030)
Thanks for pointing that out @brunoerg. Updated the commit message and PR title.

💬 MarcoFalke commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1212642042)
from CI:

```
clang: error: argument unused during compilation: '-nostdlibinc' [-Werror,-Wunused-command-line-argument]

💬 MarcoFalke commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212664168)
Is the same not true for span itself? The only reason it doesn't work is that it is missing a `std::byte` specialization?

https://github.com/bitcoin/bitcoin/blob/3a83d4417b35cb0173286b6da97315be861901bc/src/serialize.h#L202-L206

💬 MarcoFalke commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212665991)
```suggestion
DataStream value;
```

nit (feel free to ignore)

💬 MarcoFalke commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1571457996)
See for example https://github.com/achow101/wallet-fingerprinting/blob/main/fingerprints.md

💬 jonatack commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#issuecomment-1571580341)
Thank you for the review feedback, @MarcoFalke; addressed, and rebased to current master containing [`5f49cb1` (#25977)](https://github.com/bitcoin/bitcoin/pull/25977/commits/5f49cb1bc8e6ba0671c21bf6292d2d3de43fd001) to use `Result` with support for `<void>`. Invalidates previous ACK by @vasild (thanks!)

⚠️ MarcoFalke opened an issue: "fuzz: mini_miner: Timeout in mini_miner"
(https://github.com/bitcoin/bitcoin/issues/27799)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59285

_Originally posted by @MarcoFalke in https://github.com/bitcoin/bitcoin/issues/27021#issuecomment-1568242621_

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212835700)
> we should make sure that there aren't alternative clients that would ignore unrequested TXs

There is a precedent: from https://developer.bitcoin.org/reference/p2p_networking.html#tx

> [BitcoinJ](http://bitcoinj.github.io/) will send a [“tx” message](https://developer.bitcoin.org/reference/p2p_networking.html#tx) unsolicited for transactions it originates

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212837257)
For now I kept it to send `TX` right away as this is simpler and somebody blackhole-ing the transaction is ok.