💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1802795139)
Done
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1802795139)
Done
💬 darosior commented on pull request "validation: Improve input script check error reporting":
(https://github.com/bitcoin/bitcoin/pull/31097#issuecomment-2416362911)
utACK f859ff8a4e9c3aa23bf5be6eceb7099ca72b2290
(https://github.com/bitcoin/bitcoin/pull/31097#issuecomment-2416362911)
utACK f859ff8a4e9c3aa23bf5be6eceb7099ca72b2290
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802828070)
I'm bad at reading the spec, is that defined behavior? https://en.cppreference.com/w/cpp/algorithm/transform
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802828070)
I'm bad at reading the spec, is that defined behavior? https://en.cppreference.com/w/cpp/algorithm/transform
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802834385)
ok I'm convinced, removed
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802834385)
ok I'm convinced, removed
🤔 glozow reviewed a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2371973137)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2371973137)
concept ACK
💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802832478)
Given that this assert exists, I'm worried we have other places with very strict assumptions on what `IsChildWithParents` can guarantee. Perhaps it's safer to just have the RPC code or `ProcessNewPackage` to direct a single transaction to ATMP instead.
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802832478)
Given that this assert exists, I'm worried we have other places with very strict assumptions on what `IsChildWithParents` can guarantee. Perhaps it's safer to just have the RPC code or `ProcessNewPackage` to direct a single transaction to ATMP instead.
💬 1440000bytes commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-2416411781)
An alternative for anyone reading this pull request in future: https://github.com/kristapsk/bitcoin-scripts/blob/master/ricochet-send.sh
It's a bash script that uses bitcoin core wallet.
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-2416411781)
An alternative for anyone reading this pull request in future: https://github.com/kristapsk/bitcoin-scripts/blob/master/ricochet-send.sh
It's a bash script that uses bitcoin core wallet.
💬 fanquake commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2416412158)
Guix Build
```bash
d454a81e02e24f26f4d163f27ee2b16b83c8f1dfcfe0384ab544b704e27fed09 guix-build-a0e089a71dc4/output/aarch64-linux-gnu/SHA256SUMS.part
f5b3771bbb5e2c14e6d91d6a76711df0a9b8ab1b66e118eb6c5aba5686c91902 guix-build-a0e089a71dc4/output/aarch64-linux-gnu/bitcoin-a0e089a71dc4-aarch64-linux-gnu-debug.tar.gz
229f0eef5808a7266ac48b9b02dd84b7a7960353125ed7052510a8014074c0da guix-build-a0e089a71dc4/output/aarch64-linux-gnu/bitcoin-a0e089a71dc4-aarch64-linux-gnu.tar.gz
0dc7197f0284cf783
...
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2416412158)
Guix Build
```bash
d454a81e02e24f26f4d163f27ee2b16b83c8f1dfcfe0384ab544b704e27fed09 guix-build-a0e089a71dc4/output/aarch64-linux-gnu/SHA256SUMS.part
f5b3771bbb5e2c14e6d91d6a76711df0a9b8ab1b66e118eb6c5aba5686c91902 guix-build-a0e089a71dc4/output/aarch64-linux-gnu/bitcoin-a0e089a71dc4-aarch64-linux-gnu-debug.tar.gz
229f0eef5808a7266ac48b9b02dd84b7a7960353125ed7052510a8014074c0da guix-build-a0e089a71dc4/output/aarch64-linux-gnu/bitcoin-a0e089a71dc4-aarch64-linux-gnu.tar.gz
0dc7197f0284cf783
...
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802841920)
I'd rather not; we almost immediately handle singletons via `AcceptSubPackage` internally, I'm not convinced this is more risky than adding more complexity on top.
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1802841920)
I'd rather not; we almost immediately handle singletons via `AcceptSubPackage` internally, I'm not convinced this is more risky than adding more complexity on top.
💬 maflcko commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802848071)
Thanks, clarified the name, because it wasn't in a namespace itself (only the global one).
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802848071)
Thanks, clarified the name, because it wasn't in a namespace itself (only the global one).
🤔 sdaftuar reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2368652846)
Read through the code (except for the fuzz test, which I plan to go back and review). Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2368652846)
Read through the code (except for the fuzz test, which I plan to go back and review). Concept ACK.
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800758237)
Do we need this `assert()`? Can we just do an`Assume()` instead?
My general philosophy is that we should avoid calling `assert()` in codepaths that run on data received from the network, unless it really would be better for (potentially) the ENTIRE network to go down rather than just continue, in the event that the assert() can be remotely triggered.
I see that we might crash further down if we somehow there is a nullptr here, so my suggestion for error handling would be to `Assume()` eve
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800758237)
Do we need this `assert()`? Can we just do an`Assume()` instead?
My general philosophy is that we should avoid calling `assert()` in codepaths that run on data received from the network, unless it really would be better for (potentially) the ENTIRE network to go down rather than just continue, in the event that the assert() can be remotely triggered.
I see that we might crash further down if we somehow there is a nullptr here, so my suggestion for error handling would be to `Assume()` eve
...
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800930436)
I don't think it makes sense to gate this on `!bypass_limits` -- doing so only has an effect for transactions that spend a non-dust output that are coming from the same block as a transaction that creates a dust output, which seems like both (a) a very small effect that (b) we don't obviously want to do...
Specifically, it seems strange that we would only accept a transaction that was reorged out of a block that has a dust output if it has exactly 1 such dust output and exactly 0 fee, yet we
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800930436)
I don't think it makes sense to gate this on `!bypass_limits` -- doing so only has an effect for transactions that spend a non-dust output that are coming from the same block as a transaction that creates a dust output, which seems like both (a) a very small effect that (b) we don't obviously want to do...
Specifically, it seems strange that we would only accept a transaction that was reorged out of a block that has a dust output if it has exactly 1 such dust output and exactly 0 fee, yet we
...
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802693519)
nit: s/spend/spent/ ?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802693519)
nit: s/spend/spent/ ?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800908033)
I would love to merge the codepaths for the package and non-package case, so that we have no concerns over whether all the right checks have been run. What if we did something like this:
- Add all passed-in transactions to a map: Txid -> CTransactionRef. This will allow us to look up transactions either locally (from a package) or in the mempool.
- For every transaction passed in:
* For every input in the transaction:
- Look up the input in the local map. If not found, look up in
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1800908033)
I would love to merge the codepaths for the package and non-package case, so that we have no concerns over whether all the right checks have been run. What if we did something like this:
- Add all passed-in transactions to a map: Txid -> CTransactionRef. This will allow us to look up transactions either locally (from a package) or in the mempool.
- For every transaction passed in:
* For every input in the transaction:
- Look up the input in the local map. If not found, look up in
...
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802744124)
Can we assert that this transaction is different from the one already in the mempool?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802744124)
Can we assert that this transaction is different from the one already in the mempool?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802833358)
Did you mean `const CTransactionRef& tx_ref` here?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802833358)
Did you mean `const CTransactionRef& tx_ref` here?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802727338)
Just want to make sure I understand this comment, does "any size" refer to the value in the output?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802727338)
Just want to make sure I understand this comment, does "any size" refer to the value in the output?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802745380)
Might be nice to assert that this transaction has a higher feerate than the one in the mempool, just to be sure that this would be a successful RBF if not for the ephemeral dust issue.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1802745380)
Might be nice to assert that this transaction has a higher feerate than the one in the mempool, just to be sure that this would be a successful RBF if not for the ephemeral dust issue.
💬 dergoegge commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2416431400)
I opened #31093, implementing the suggestion from https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400559220.
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2416431400)
I opened #31093, implementing the suggestion from https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400559220.