💬 brunoerg commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570544534)
I implemented this for testing purposes which have been very useful for me and could be for other reviewers, initially my idea is not open this, just did it after seeing https://github.com/bitcoin/bitcoin/issues/26602. However, I agree with @mzumsande about annoyability of removing fields from RPC, so I can leave this as "draft" for more discussions/not being merged and hopefully these commits can be picked in #21515 and then we can have it.
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570544534)
I implemented this for testing purposes which have been very useful for me and could be for other reviewers, initially my idea is not open this, just did it after seeing https://github.com/bitcoin/bitcoin/issues/26602. However, I agree with @mzumsande about annoyability of removing fields from RPC, so I can leave this as "draft" for more discussions/not being merged and hopefully these commits can be picked in #21515 and then we can have it.
📝 brunoerg converted_to_draft a pull request: "rpc, net: add erlay status in `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/27797)
Fixes #26602
Adds `m_tx_reconciliation` in `Peer` struct
to know whether the peer supports Erlay and
exposes it in `getpeerinfo` rpc.
(https://github.com/bitcoin/bitcoin/pull/27797)
Fixes #26602
Adds `m_tx_reconciliation` in `Peer` struct
to know whether the peer supports Erlay and
exposes it in `getpeerinfo` rpc.
🤔 zkfrio requested changes to a pull request: "wallet: Deniability API (Unilateral Transaction Meta-Privacy)"
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1453686280)
NACK
It doesn't improve privacy. Not considering fingerprints. A better way to improve privacy in bitcoin payments using bitcoin core could be payjoin.
False sense of privacy implemented in this pull request only helps miners with fees.
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1453686280)
NACK
It doesn't improve privacy. Not considering fingerprints. A better way to improve privacy in bitcoin payments using bitcoin core could be payjoin.
False sense of privacy implemented in this pull request only helps miners with fees.
💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570556203)
> The commits can be improved, but currently good enough for (non-guix) testing / discussion.
What are plans regarding Guix builds?
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570556203)
> The commits can be improved, but currently good enough for (non-guix) testing / discussion.
What are plans regarding Guix builds?
💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570559259)
> What are plans regarding Guix builds?
What you've quoted was just an outdated part of the PR description, that I've not dropped.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570559259)
> What are plans regarding Guix builds?
What you've quoted was just an outdated part of the PR description, that I've not dropped.
💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570563184)
> > What are plans regarding Guix builds?
>
> What you've quoted was just an outdated part of the PR description, that I've now dropped.
Well, then Guix does not produced unsigned `*-apple-darwin-unsigned.zip`, instead of `*-apple-darwin-unsigned.dmg`:
```
1bed850e74f985e3db277160529aff767f4d3be00822ee8e204cdff304664727 guix-build-bec052f9ac0b/output/arm64-apple-darwin/SHA256SUMS.part
6b3a93b33d163dcc6a1a8582c3db1fe2c828620757c078e0c59f3e1b1eee3990 guix-build-bec052f9ac0b/output/arm64
...
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570563184)
> > What are plans regarding Guix builds?
>
> What you've quoted was just an outdated part of the PR description, that I've now dropped.
Well, then Guix does not produced unsigned `*-apple-darwin-unsigned.zip`, instead of `*-apple-darwin-unsigned.dmg`:
```
1bed850e74f985e3db277160529aff767f4d3be00822ee8e204cdff304664727 guix-build-bec052f9ac0b/output/arm64-apple-darwin/SHA256SUMS.part
6b3a93b33d163dcc6a1a8582c3db1fe2c828620757c078e0c59f3e1b1eee3990 guix-build-bec052f9ac0b/output/arm64
...
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1570566067)
The first two commits up to f50fa248f1d904be5f33a29c837f74a2cca8abc0 look good to me.
How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
> Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
In any case moving `AcceptBlock` from `Chainstate` to `ChainstateManager` seems an improvement.
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1570566067)
The first two commits up to f50fa248f1d904be5f33a29c837f74a2cca8abc0 look good to me.
How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
> Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
In any case moving `AcceptBlock` from `Chainstate` to `ChainstateManager` seems an improvement.
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1212012110)
Removed it, thanks!
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1212012110)
Removed it, thanks!
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1570569762)
Force-pushed removing the unused variable `random_mutable_transaction`
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1570569762)
Force-pushed removing the unused variable `random_mutable_transaction`
💬 hebasto commented on pull request "Fix transaction view/table":
(https://github.com/bitcoin-core/gui/pull/662#issuecomment-1570569786)
Closing due to a long period of inactivity here. Feel free to reopen.
(https://github.com/bitcoin-core/gui/pull/662#issuecomment-1570569786)
Closing due to a long period of inactivity here. Feel free to reopen.
✅ hebasto closed a pull request: "Fix transaction view/table"
(https://github.com/bitcoin-core/gui/pull/662)
(https://github.com/bitcoin-core/gui/pull/662)
💬 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-1570613820)
> > just need to decide out what to do about guix.
>
> What options are currently being considered. Is an upstream patch still possible?
Yes, I'm still working on testing this. It changes how the llvm build works and I'd like to be more confident in the change before proposing it.
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1570613820)
> > just need to decide out what to do about guix.
>
> What options are currently being considered. Is an upstream patch still possible?
Yes, I'm still working on testing this. It changes how the llvm build works and I'd like to be more confident in the change before proposing it.
💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1570613994)
> Bitcoin Core's wallet has a "unique" fingerprint on transactions that is shared by way less than 50% of transactions in the network ...
By fingerprint, do you mean Bitcoin Core's transactions can be identified on the block chain? If that's the case that's unfortunate for sure. What's the nature of the difference? Is it on purpose and if not, would it be possible to fix?
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1570613994)
> Bitcoin Core's wallet has a "unique" fingerprint on transactions that is shared by way less than 50% of transactions in the network ...
By fingerprint, do you mean Bitcoin Core's transactions can be identified on the block chain? If that's the case that's unfortunate for sure. What's the nature of the difference? Is it on purpose and if not, would it be possible to fix?
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570638653)
> Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.
Oh you're right. That seems like a better idea, simpler than either of my suggestions. And I think it could allow getting rid of the `vRPCConvertParams` table entirely?
I think the only possible downside of having the server automatically convert unexpected string parameters to JSON numbers/bools/objects would be if
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570638653)
> Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.
Oh you're right. That seems like a better idea, simpler than either of my suggestions. And I think it could allow getting rid of the `vRPCConvertParams` table entirely?
I think the only possible downside of having the server automatically convert unexpected string parameters to JSON numbers/bools/objects would be if
...
💬 achow101 commented on pull request "index: prevent race by calling 'CustomInit' prior setting 'synced' flag":
(https://github.com/bitcoin/bitcoin/pull/27720#issuecomment-1570652628)
ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58
(https://github.com/bitcoin/bitcoin/pull/27720#issuecomment-1570652628)
ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570654857)
@ryanofsky Are you planning on making any further changes here?
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570654857)
@ryanofsky Are you planning on making any further changes here?
💬 sr-gi commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1570662645)
So just to be clear, my concern here relates to the fact that the current codebase only allows a peer to request a transaction as long as we have announced that to them (with the exception of data requested after a `MEMPOOL` message if we happen run signaling bip37).
After this patch, the logic above won't hold true anymore. If for whatever reason we have more than `INV_BROADCAST_MAX` transactions to send to a given peer at a given time, and we send an INV message to it, it will immediately b
...
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1570662645)
So just to be clear, my concern here relates to the fact that the current codebase only allows a peer to request a transaction as long as we have announced that to them (with the exception of data requested after a `MEMPOOL` message if we happen run signaling bip37).
After this patch, the logic above won't hold true anymore. If for whatever reason we have more than `INV_BROADCAST_MAX` transactions to send to a given peer at a given time, and we send an INV message to it, it will immediately b
...
💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1570668159)
I get this when compiling with both gcc and clang-16. I am also using libevent 2.1.12. Is there anything unique about your setup @willcl-ark ? Also, did you Ctrl-C the `waitforblockheight` command before trying to stop bitcoind?
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1570668159)
I get this when compiling with both gcc and clang-16. I am also using libevent 2.1.12. Is there anything unique about your setup @willcl-ark ? Also, did you Ctrl-C the `waitforblockheight` command before trying to stop bitcoind?
🚀 achow101 merged a pull request: "index: prevent race by calling 'CustomInit' prior setting 'synced' flag"
(https://github.com/bitcoin/bitcoin/pull/27720)
(https://github.com/bitcoin/bitcoin/pull/27720)
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212094169)
8990668: it would be useful to have radd and rsub back for [these operations in `xswiftec_inv`](https://github.com/bitcoin/bips/blob/master/bip-0324/reference.py#L367-L370).
(to keep #24005 consistent with BIP's reference python implementation for easy review.)
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212094169)
8990668: it would be useful to have radd and rsub back for [these operations in `xswiftec_inv`](https://github.com/bitcoin/bips/blob/master/bip-0324/reference.py#L367-L370).
(to keep #24005 consistent with BIP's reference python implementation for easy review.)