💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3549979018)
> > The first three PRs were already part of #28792, the others are new.
>
> i'm sure you mean the first three _commits_? 😄
Yepp, fixed :)
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3549979018)
> > The first three PRs were already part of #28792, the others are new.
>
> i'm sure you mean the first three _commits_? 😄
Yepp, fixed :)
💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2540013317)
The commit is here because this PR depends on #33026 and the commit is coming from there, so the discussion on these commits should be held there. I just responded there, so marking this as resolved here.
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2540013317)
The commit is here because this PR depends on #33026 and the commit is coming from there, so the discussion on these commits should be held there. I just responded there, so marking this as resolved here.
🤔 w0xlt reviewed a pull request: "kernel: Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891#pullrequestreview-3480249411)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33891#pullrequestreview-3480249411)
Concept ACK
💬 ajtowns commented on pull request "[wip] wallet: Add separate balance info for non-mempool wallet txs":
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3550142278)
> This extends the `getbalances` RPC. Would be good to also update the GUI (not necessary in this PR).
Okay, I have a patch for that at https://github.com/ajtowns/bitcoin/commits/202511-wallet-unconf-bal-gui/
> The new type of transactions would have somehow to be fed to the wallet so that they would be returned by `wallet.GetTXOs()`, called by `GetBalance()`. How would that be done?
I don't think there's a way to do this for arbitrary txs that originate outside of the wallet, even if t
...
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3550142278)
> This extends the `getbalances` RPC. Would be good to also update the GUI (not necessary in this PR).
Okay, I have a patch for that at https://github.com/ajtowns/bitcoin/commits/202511-wallet-unconf-bal-gui/
> The new type of transactions would have somehow to be fed to the wallet so that they would be returned by `wallet.GetTXOs()`, called by `GetBalance()`. How would that be done?
I don't think there's a way to do this for arbitrary txs that originate outside of the wallet, even if t
...
👋 ajtowns's pull request is ready for review: "[wip] wallet: Add separate balance info for non-mempool wallet txs"
(https://github.com/bitcoin/bitcoin/pull/33671)
(https://github.com/bitcoin/bitcoin/pull/33671)
📝 ajtowns opened a pull request: "Adds non-mempool wallet balance to overview"
(https://github.com/bitcoin-core/gui/pull/911)
The wallet can contain transactions that are not accepted into the node's mempool (eg due to containing a too large OP_RETURN output, due to too low a feerate, or due to too many unconfirmed ancestors). In the event you end up in this situation, it can appear as if funds have gone missing from your wallet due to the non-mempool balance not being reported. Correct this by reporting the non-mempool balance.
Depends on bitcoin/bitcoin#33671
(https://github.com/bitcoin-core/gui/pull/911)
The wallet can contain transactions that are not accepted into the node's mempool (eg due to containing a too large OP_RETURN output, due to too low a feerate, or due to too many unconfirmed ancestors). In the event you end up in this situation, it can appear as if funds have gone missing from your wallet due to the non-mempool balance not being reported. Correct this by reporting the non-mempool balance.
Depends on bitcoin/bitcoin#33671
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3550159408)
> I do think the PR could use a more detailed description.
Should I cut and paste what you wrote below into the PR or commit description? Sorry, I need an ELI5 here :)
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3550159408)
> I do think the PR could use a more detailed description.
Should I cut and paste what you wrote below into the PR or commit description? Sorry, I need an ELI5 here :)
💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3550406321)
> This checking m ≤ n behaviour in the Solver seems to have been added in [#13194](https://github.com/bitcoin/bitcoin/pull/13194). However this wouldn't count as soft-confiscations because multisigs with m > n are presumably unspendable anways.
Yeah, see https://github.com/bitcoin/bitcoin/blob/4405b78d6059e536c36974088a8ed4d9f0f29898/script.cpp#L745 from the start of git history.
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3550406321)
> This checking m ≤ n behaviour in the Solver seems to have been added in [#13194](https://github.com/bitcoin/bitcoin/pull/13194). However this wouldn't count as soft-confiscations because multisigs with m > n are presumably unspendable anways.
Yeah, see https://github.com/bitcoin/bitcoin/blob/4405b78d6059e536c36974088a8ed4d9f0f29898/script.cpp#L745 from the start of git history.
💬 waketraindev commented on pull request "[30.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3550668156)
Please add https://github.com/bitcoin-core/gui/pull/910 , it's test coverage for https://github.com/bitcoin-core/gui/pull/901
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3550668156)
Please add https://github.com/bitcoin-core/gui/pull/910 , it's test coverage for https://github.com/bitcoin-core/gui/pull/901
💬 waketraindev commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3550854849)
ACK 8558902e576e2c2d66f6083b66953dd6cc464de4
Tested on Windows 11 (Dark Theme) (wsl/depends cross build) x86_64-w64-mingw32 [Release config]
Tested on Windows 11 (Dark Theme) (guix build) x86_64-w64-mingw32
<img width="781" height="161" alt="image" src="https://github.com/user-attachments/assets/9400b604-86a5-4635-9fa5-07e21b80e32e" />
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3550854849)
ACK 8558902e576e2c2d66f6083b66953dd6cc464de4
Tested on Windows 11 (Dark Theme) (wsl/depends cross build) x86_64-w64-mingw32 [Release config]
Tested on Windows 11 (Dark Theme) (guix build) x86_64-w64-mingw32
<img width="781" height="161" alt="image" src="https://github.com/user-attachments/assets/9400b604-86a5-4635-9fa5-07e21b80e32e" />
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2540645348)
Yes. Maybe some stuff from here can be moved to `Socks5Configuration` and/or `Socks5Server`. Maybe the SOCKS5 server can have a default "destinations factory" function which does what is in the `else` branch here. Then there could be callback which either tells where to redirect the Nth connection or tells to use the default
```
default destinations factory:
# let i be the number of the connection
if override callback is set:
whereto = override(i)
if whereto is no
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2540645348)
Yes. Maybe some stuff from here can be moved to `Socks5Configuration` and/or `Socks5Server`. Maybe the SOCKS5 server can have a default "destinations factory" function which does what is in the `else` branch here. Then there could be callback which either tells where to redirect the Nth connection or tells to use the default
```
default destinations factory:
# let i be the number of the connection
if override callback is set:
whereto = override(i)
if whereto is no
...
💬 ajtowns commented on pull request "Adds non-mempool wallet balance to overview":
(https://github.com/bitcoin-core/gui/pull/911#issuecomment-3551114643)
To test, start with `-datacarriersize=10`, and run
```
send '{"data": "4368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73"}' null "unset" 1
```
from the console. "Non-mempool: <change amount>" should appear in the Balances pane on the Overview window with the change from the transaction. Restarting with `-datacarriersize=100000` should allow the tx to enter the mempool, and move the amount into the Available balance. (Presumably, do this on -re
...
(https://github.com/bitcoin-core/gui/pull/911#issuecomment-3551114643)
To test, start with `-datacarriersize=10`, and run
```
send '{"data": "4368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73"}' null "unset" 1
```
from the console. "Non-mempool: <change amount>" should appear in the Balances pane on the Overview window with the change from the transaction. Restarting with `-datacarriersize=100000` should allow the tx to enter the mempool, and move the amount into the Available balance. (Presumably, do this on -re
...
💬 hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3551271373)
My Guix build:
```
aarch64
15ff16a4958d217927fb68c99c0dcd19bf51595517c59aae8b442b3e1b4529e2 guix-build-8558902e576e/output/aarch64-linux-gnu/SHA256SUMS.part
db8080232c5216f0f9cb06ce7052fe81156f9dab6a15964be187f1186f24e47e guix-build-8558902e576e/output/aarch64-linux-gnu/bitcoin-8558902e576e-aarch64-linux-gnu-debug.tar.gz
e3c053d21dcab9234a1943af0e61a2c7c9270429238e0eb11fb0378378bcf87e guix-build-8558902e576e/output/aarch64-linux-gnu/bitcoin-8558902e576e-aarch64-linux-gnu.tar.gz
1194b7e6
...
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3551271373)
My Guix build:
```
aarch64
15ff16a4958d217927fb68c99c0dcd19bf51595517c59aae8b442b3e1b4529e2 guix-build-8558902e576e/output/aarch64-linux-gnu/SHA256SUMS.part
db8080232c5216f0f9cb06ce7052fe81156f9dab6a15964be187f1186f24e47e guix-build-8558902e576e/output/aarch64-linux-gnu/bitcoin-8558902e576e-aarch64-linux-gnu-debug.tar.gz
e3c053d21dcab9234a1943af0e61a2c7c9270429238e0eb11fb0378378bcf87e guix-build-8558902e576e/output/aarch64-linux-gnu/bitcoin-8558902e576e-aarch64-linux-gnu.tar.gz
1194b7e6
...
💬 vasild commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3551296046)
`prepareTransaction()` is called before "Create Unsigned" is clicked. It seems a bit confusing, there are two "Send" buttons.
The user selects inputs, fills destination address and amount and clicks "Send". This is when `SendCoinsDialog::sendButtonClicked()` is called. Then it calls `SendCoinsDialog::PrepareSendText() -> WalletModel::prepareTransaction()`. At this point the transaction is created and signed. I think it must be in order to calculate the fee exactly? Then a dialog is presented "D
...
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3551296046)
`prepareTransaction()` is called before "Create Unsigned" is clicked. It seems a bit confusing, there are two "Send" buttons.
The user selects inputs, fills destination address and amount and clicks "Send". This is when `SendCoinsDialog::sendButtonClicked()` is called. Then it calls `SendCoinsDialog::PrepareSendText() -> WalletModel::prepareTransaction()`. At this point the transaction is created and signed. I think it must be in order to calculate the fee exactly? Then a dialog is presented "D
...
💬 maflcko commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3551316361)
review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a 🏀
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 7f318e1dd049
...
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3551316361)
review ACK 7f318e1dd0496384e7bc6d8754c5a2a618a14b2a 🏀
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 7f318e1dd049
...
💬 maflcko commented on pull request "rpc: Add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3551325875)
the commit message is wrong?
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3551325875)
the commit message is wrong?
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3551348815)
Latest push rebased to resolve conflict with #31734. + Added explicit test coverage of `operator=(Node&&)` (https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3490078075).
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3551348815)
Latest push rebased to resolve conflict with #31734. + Added explicit test coverage of `operator=(Node&&)` (https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3490078075).
💬 Sjors commented on issue "rfc: virtio-vsock for RPC and IPC":
(https://github.com/bitcoin/bitcoin/issues/33897#issuecomment-3551516882)
> it would not really be safe without authentication
And encryption, since we can't use SSL.
(https://github.com/bitcoin/bitcoin/issues/33897#issuecomment-3551516882)
> it would not really be safe without authentication
And encryption, since we can't use SSL.
💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3551580243)
@plebhash indeed I'm not worried about client-side memory, it's node that could get into memory trouble if there's a long block interval with lots of mempool churn.
> it's extremely unlikely that mempool would sustain sub-second changes for over 2h
In the context of templates we only care about about at the top of the mempool. For the mempool as a whole there can be dozens of transactions per second, see e.g. #28592.
@ryanofsky how would the node go about tracking the memory footprint of clie
...
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3551580243)
@plebhash indeed I'm not worried about client-side memory, it's node that could get into memory trouble if there's a long block interval with lots of mempool churn.
> it's extremely unlikely that mempool would sustain sub-second changes for over 2h
In the context of templates we only care about about at the top of the mempool. For the mempool as a whole there can be dozens of transactions per second, see e.g. #28592.
@ryanofsky how would the node go about tracking the memory footprint of clie
...
💬 Sjors commented on pull request "mining: add requestedOutputs field, e.g. for merged mining":
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3551630141)
> is this aimed at Sv2?
Any consumer of the interface, but I mostly have Stratum v2 in mind.
> if it requires a spec change so this can be integrated
As @ajtowns points out in the https://github.com/stratum-mining/sv2-spec/issues/167 thread it's a bit odd that @TheBlueMatt states that merged-mining is "a key goal of Sv2" while the spec doesn't even mention it.
In principle the change here doesn't need a spec change. Miners who want merged-mining will patch _something_, and this just
...
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3551630141)
> is this aimed at Sv2?
Any consumer of the interface, but I mostly have Stratum v2 in mind.
> if it requires a spec change so this can be integrated
As @ajtowns points out in the https://github.com/stratum-mining/sv2-spec/issues/167 thread it's a bit odd that @TheBlueMatt states that merged-mining is "a key goal of Sv2" while the spec doesn't even mention it.
In principle the change here doesn't need a spec change. Miners who want merged-mining will patch _something_, and this just
...