👋 vasild's pull request is ready for review: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509)
(https://github.com/bitcoin/bitcoin/pull/27509)
💬 fanquake commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212894727)
`These results, as well as those of -addrinfo, are filtered for quality and recency.` - can you drop this. It's not clear why we need to mention an implemention detail of `-addrinfo` here.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212894727)
`These results, as well as those of -addrinfo, are filtered for quality and recency.` - can you drop this. It's not clear why we need to mention an implemention detail of `-addrinfo` here.
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212736489)
ee6e289:
```suggestion
def __rsub__(self, a):
```
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212736489)
ee6e289:
```suggestion
def __rsub__(self, a):
```
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212950016)
> The way FE values are represented inside the class is an unobservable implementation detail, so calling int(FE) doesn't "update" the FE object in any observable way - it just makes future calls more efficient.
thinking of it this way makes sense. thanks!
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212950016)
> The way FE values are represented inside the class is an unobservable implementation detail, so calling int(FE) doesn't "update" the FE object in any observable way - it just makes future calls more efficient.
thinking of it this way makes sense. thanks!
💬 brandonpille commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1571814230)
No it's ok
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1571814230)
No it's ok
✅ fanquake closed an issue: "rpc: Allow importing wallets by data instead of by filename"
(https://github.com/bitcoin/bitcoin/issues/27597)
(https://github.com/bitcoin/bitcoin/issues/27597)
💬 MarnixCroes commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212994429)
done
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212994429)
done
💬 theuni commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1213032495)
ffs...
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1213032495)
ffs...
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213064407)
> It's not clear why we need to mention an implemention detail of `-addrinfo` here.
That is how RPC `getnodeaddresses` works as well -- see its help.
Would suggest either reverting to the previous push, or replacing this paragraph in tor/i2p/cjdns.md with something like the following:
You can use the `getnodeaddresses` RPC to fetch a number of onion peers known to your node; run `bitcoin-cli help getnodeaddresses` for details.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213064407)
> It's not clear why we need to mention an implemention detail of `-addrinfo` here.
That is how RPC `getnodeaddresses` works as well -- see its help.
Would suggest either reverting to the previous push, or replacing this paragraph in tor/i2p/cjdns.md with something like the following:
You can use the `getnodeaddresses` RPC to fetch a number of onion peers known to your node; run `bitcoin-cli help getnodeaddresses` for details.
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213069877)
Now that the link has been removed, please update the PR title and description from "fix" to "remove".
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213069877)
Now that the link has been removed, please update the PR title and description from "fix" to "remove".
💬 glozow commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1571967192)
> Because a -fallback fee already exists. Why can't it exist for other use cases?
Node and wallet are 2 separate components; it doesn't makes sense for a wallet option to bleed into node behavior. Another example: the wallet setting `-walletrejectlongchains=0` does not mean the mempool should also accept long chains of transactions, and the existence of that wallet option does not mean we should also add that option to the node. If inheritance exists, the wallet's options should be informed b
...
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1571967192)
> Because a -fallback fee already exists. Why can't it exist for other use cases?
Node and wallet are 2 separate components; it doesn't makes sense for a wallet option to bleed into node behavior. Another example: the wallet setting `-walletrejectlongchains=0` does not mean the mempool should also accept long chains of transactions, and the existence of that wallet option does not mean we should also add that option to the node. If inheritance exists, the wallet's options should be informed b
...
💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1571998659)
Side note: I tried centos and it also failed with the same error as debian. Commit:
<details><summary>diff</summary>
```diff
commit 57903b154ec1dc5d86f174d311e9875bbf0c4106
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Thu Jun 1 13:48:41 2023 +0200
ci: Use centos to work around fuzz msan debian bug
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..5aca7a9be9 100755
--- a/ci/test/00_setu
...
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1571998659)
Side note: I tried centos and it also failed with the same error as debian. Commit:
<details><summary>diff</summary>
```diff
commit 57903b154ec1dc5d86f174d311e9875bbf0c4106
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Thu Jun 1 13:48:41 2023 +0200
ci: Use centos to work around fuzz msan debian bug
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..5aca7a9be9 100755
--- a/ci/test/00_setu
...
💬 fanquake commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213129896)
> You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.
If anything, this seems better than reverting to the previous text.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213129896)
> You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.
If anything, this seems better than reverting to the previous text.
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213130776)
Agree.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213130776)
Agree.
💬 dergoegge commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1572044611)
utACK 5763b232e6e6a0f72d046f8aa322b39328be135b
Looking forward to green check marks on qa-assets
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1572044611)
utACK 5763b232e6e6a0f72d046f8aa322b39328be135b
Looking forward to green check marks on qa-assets
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213206571)
> Is the same not true for span itself?
No because `Span<char>` is fixed length and can be deserialized, while `DataStream` is variable length and can't be deserialized.
Reasons why I think it would be good to get rid of `DataStream::Serialize` method:
- `DataStream` has no `Unserialize` method and no reasonable way of adding one that would be consistent with the existing `Serialize` implementation.
- The `Serialize` method has surprising behavior. If `DataStream` was going to have a
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213206571)
> Is the same not true for span itself?
No because `Span<char>` is fixed length and can be deserialized, while `DataStream` is variable length and can't be deserialized.
Reasons why I think it would be good to get rid of `DataStream::Serialize` method:
- `DataStream` has no `Unserialize` method and no reasonable way of adding one that would be consistent with the existing `Serialize` implementation.
- The `Serialize` method has surprising behavior. If `DataStream` was going to have a
...
💬 darosior commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572121111)
> When scripts are decoded into ASM, two different integers can be displayed identically, with one as hex and the other as decimal.
It's not two different integers, it's that integers (which all <=4 bytes pushes are assumed to be) are displayed in decimal and all other pushes are displayed as hexadecimal.
I wonder why we even try to detect and print numbers differently (and even more so in a different base!), just print all pushes in hex and there is no ambiguity anymore?
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572121111)
> When scripts are decoded into ASM, two different integers can be displayed identically, with one as hex and the other as decimal.
It's not two different integers, it's that integers (which all <=4 bytes pushes are assumed to be) are displayed in decimal and all other pushes are displayed as hexadecimal.
I wonder why we even try to detect and print numbers differently (and even more so in a different base!), just print all pushes in hex and there is no ambiguity anymore?
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572135299)
> Approach review is very welcome on this PR
didn't real whole OP until now. In my not expert opinion, the orphan handling seems reasonable, and I think it makes to open it ASAP to take it out of draft
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572135299)
> Approach review is very welcome on this PR
didn't real whole OP until now. In my not expert opinion, the orphan handling seems reasonable, and I think it makes to open it ASAP to take it out of draft
👍 fanquake approved a pull request: "doc: remove Tor link & generalize onion getnodeaddresses RPC"
(https://github.com/bitcoin/bitcoin/pull/27719#pullrequestreview-1455587440)
ACK 6fce5ddc17ac9d1e07849f92088ea3f7cfcafe26
(https://github.com/bitcoin/bitcoin/pull/27719#pullrequestreview-1455587440)
ACK 6fce5ddc17ac9d1e07849f92088ea3f7cfcafe26
💬 MatthewLM commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572151288)
They are two different numbers. One is `0x1112131415` and the other is `0x57c74942`. In ASM, they are displayed the same. Using `0x` before hex is consistent with hex literals elsewhere and would allow for decimals to be displayed including `-1` for `OP_1NEGATE`.
Displaying all numbers as hex is an option. One problem is that they are given as little-endian which could be confusing for some wishing to interpret them. Off the top of my memory, the scripting interpreter treats 32-bit numbers ar
...
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572151288)
They are two different numbers. One is `0x1112131415` and the other is `0x57c74942`. In ASM, they are displayed the same. Using `0x` before hex is consistent with hex literals elsewhere and would allow for decimals to be displayed including `-1` for `OP_1NEGATE`.
Displaying all numbers as hex is an option. One problem is that they are given as little-endian which could be confusing for some wishing to interpret them. Off the top of my memory, the scripting interpreter treats 32-bit numbers ar
...