💬 furszy commented on pull request "wallet: finish addressbook encapsulation and simplify addressbook migration":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1570466415)
rebased
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1570466415)
rebased
💬 Sjors commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570469083)
@mzumsande given that we have `-txreconciliation` I think it's fine to add this. We can always drop it if Erlay is given up on. Making it easier to test, makes it more likely to ever get finished though.
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570469083)
@mzumsande given that we have `-txreconciliation` I think it's fine to add this. We can always drop it if Erlay is given up on. Making it easier to test, makes it more likely to ever get finished though.
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570475171)
> One alternative could be to avoid the need to process arguments on the client side at all and and add a new `exec` RPC method that takes an `args` array of strings and an optional `stdin` string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An `exec` method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new `-exec` or `-dumb` option to invok
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570475171)
> One alternative could be to avoid the need to process arguments on the client side at all and and add a new `exec` RPC method that takes an `args` array of strings and an optional `stdin` string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An `exec` method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new `-exec` or `-dumb` option to invok
...
💬 hebasto 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-1570487179)
> just need to decide out what to do about guix.
What options are currently being considered. Is an upstream patch still possible?
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1570487179)
> just need to decide out what to do about guix.
What options are currently being considered. Is an upstream patch still possible?
💬 mzumsande commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570503893)
> given that we have `-txreconciliation` I think it's fine to add this.
But `-txreconciliation` is off-by default and not exposed to users, this field exposes Erlay-related info unconditionally in a popular RPC, which could create confusion (why is this field there if I can't turn Erlay on yet?) and seems premature given that Erlay could still be years away (if it makes it at all, which I do hope!). So I'd prefer it if these commits were part of #21515 (which anyone wanting to actually test E
...
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570503893)
> given that we have `-txreconciliation` I think it's fine to add this.
But `-txreconciliation` is off-by default and not exposed to users, this field exposes Erlay-related info unconditionally in a popular RPC, which could create confusion (why is this field there if I can't turn Erlay on yet?) and seems premature given that Erlay could still be years away (if it makes it at all, which I do hope!). So I'd prefer it if these commits were part of #21515 (which anyone wanting to actually test E
...
💬 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