💬 dergoegge commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830823046)
Perhaps in a follow up we can make mocktime accurate to the millisecond and then advance it here to simulate an actual sleep.
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830823046)
Perhaps in a follow up we can make mocktime accurate to the millisecond and then advance it here to simulate an actual sleep.
👍 laanwj approved a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216#pullrequestreview-2417975546)
ACK 97235c446e9986ecca09c2a4b78d6c6239853fdb97235c446e9986ecca09c2a4b78d6c6239853fdb
good to have the secp256k1_memclear commits
(https://github.com/bitcoin/bitcoin/pull/31216#pullrequestreview-2417975546)
ACK 97235c446e9986ecca09c2a4b78d6c6239853fdb97235c446e9986ecca09c2a4b78d6c6239853fdb
good to have the secp256k1_memclear commits
💬 fanquake commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2459458747)
Guix Build:
```bash
df8b274bbadabebfc2ec3d28591f6f66a983f2500779fb95016aaef8c728ad8d guix-build-97235c446e99/output/aarch64-linux-gnu/SHA256SUMS.part
26aeeb60bf02b1d6e1c072a5c42a3ec14165f208efb5f17992f1716c2db7b826 guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu-debug.tar.gz
043e16b13eed1ff5b3a1c4ce67309d595fffbdbbd0cd6800221a9eabb84a35eb guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu.tar.gz
3f2b606211c543d8
...
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2459458747)
Guix Build:
```bash
df8b274bbadabebfc2ec3d28591f6f66a983f2500779fb95016aaef8c728ad8d guix-build-97235c446e99/output/aarch64-linux-gnu/SHA256SUMS.part
26aeeb60bf02b1d6e1c072a5c42a3ec14165f208efb5f17992f1716c2db7b826 guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu-debug.tar.gz
043e16b13eed1ff5b3a1c4ce67309d595fffbdbbd0cd6800221a9eabb84a35eb guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu.tar.gz
3f2b606211c543d8
...
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1830836157)
I think this is worth a followup. If you agree, I would open one, or just leave it as it is? What to reword it to? "Ending handling of connection to ..."? Or log different messages based on `self.serv.keep_alive`?
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1830836157)
I think this is worth a followup. If you agree, I would open one, or just leave it as it is? What to reword it to? "Ending handling of connection to ..."? Or log different messages based on `self.serv.keep_alive`?
📝 fanquake opened a pull request: "ci: `add second_deadlock_stack=1` to TSAN options"
(https://github.com/bitcoin/bitcoin/pull/31232)
This is mentioned in the developer notes, but isn't present in `TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures occur. Checked that adding does output mor
...
(https://github.com/bitcoin/bitcoin/pull/31232)
This is mentioned in the developer notes, but isn't present in `TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures occur. Checked that adding does output mor
...
🚀 fanquake merged a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216)
(https://github.com/bitcoin/bitcoin/pull/31216)
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090)
The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE` (and I'd assume `PSBT_IN_MUSIG2_PARTIAL_SIGNATURE`, but I didn't reach there, yet); instead, [BIP-373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) says that it must be _the key found in the script and not the aggregate public key that it was derived from, if it was derived from an aggregate key_. Therefore, I interpreted it as the taproot pubk
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090)
The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE` (and I'd assume `PSBT_IN_MUSIG2_PARTIAL_SIGNATURE`, but I didn't reach there, yet); instead, [BIP-373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) says that it must be _the key found in the script and not the aggregate public key that it was derived from, if it was derived from an aggregate key_. Therefore, I interpreted it as the taproot pubk
...
🤔 rkrux reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830700460)
Although it seems correct to run the loop for the outputs of the parent transaction but as per the definition of Ephemeral Dust, we can break (and end) the loop here after the dust is found because only one is allowed per parent transaction?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830700460)
Although it seems correct to run the loop for the outputs of the parent transaction but as per the definition of Ephemeral Dust, we can break (and end) the loop here after the dust is found because only one is allowed per parent transaction?
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830720982)
Overall this is a pretty neat function that I enjoyed going through!
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830720982)
Overall this is a pretty neat function that I enjoyed going through!
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830693824)
> Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee.
As per this definition, shouldn't this function also check that there is only one dust output in the transaction? I can see it's checked in the `IsStandardTx()` but IMO this function that should check the complete validity of the ephemeral transaction should encapsulate this single dust output check as well.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830693824)
> Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee.
As per this definition, shouldn't this function also check that there is only one dust output in the transaction? I can see it's checked in the `IsStandardTx()` but IMO this function that should check the complete validity of the ephemeral transaction should encapsulate this single dust output check as well.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830774549)
With this change, `IsStandardTx()` allows having only 1 dust output per tx and makes such txs technically standard but without any constraint of the 0-fee part that comes later down in the `PreChecks()`. Although there are no other usages of the `IsStandardTx()` in the source code (besides the CPP tests) but if later usages do arise, they would miss out on the 0-fee check.
TL;DR: The 0-fee check and the 1-dust output check that are tied by the Ephemeral Dust concept don't exist together within
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830774549)
With this change, `IsStandardTx()` allows having only 1 dust output per tx and makes such txs technically standard but without any constraint of the 0-fee part that comes later down in the `PreChecks()`. Although there are no other usages of the `IsStandardTx()` in the source code (besides the CPP tests) but if later usages do arise, they would miss out on the 0-fee check.
TL;DR: The 0-fee check and the 1-dust output check that are tied by the Ephemeral Dust concept don't exist together within
...
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830784855)
> Priority is not supported for transactions with dust outputs.
Nit: IIUC, there should not be more than 1 dust output per transaction but this reads otherwise.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830784855)
> Priority is not supported for transactions with dust outputs.
Nit: IIUC, there should not be more than 1 dust output per transaction but this reads otherwise.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830778947)
There is a underlying assumption here that this constant is used only for the ephemeral dust use case but it is not evident in the naming of this constant. Related to the previous comment on `IsStandardTx()`.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830778947)
There is a underlying assumption here that this constant is used only for the ephemeral dust use case but it is not evident in the naming of this constant. Related to the previous comment on `IsStandardTx()`.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830898538)
> Must be called for each transaction(package) if any dust is in the package. Checks that each transaction's parents have their dust spent by the child,
Should this loop run only for dust inputs by adding a `IsDust()` right at the beginning of this loop? I'm assuming this function is not responsible for the standard transaction validity checks as per the comment in the header file.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830898538)
> Must be called for each transaction(package) if any dust is in the package. Checks that each transaction's parents have their dust spent by the child,
Should this loop run only for dust inputs by adding a `IsDust()` right at the beginning of this loop? I'm assuming this function is not responsible for the standard transaction validity checks as per the comment in the header file.
💬 glozow commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459565875)
@dergoegge is that on master, or with the changes in this PR? Would like to see if it could be a negative time thing.
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459565875)
@dergoegge is that on master, or with the changes in this PR? Would like to see if it could be a negative time thing.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830902499)
Aah nvm, `IsDust` works on TxOuts.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830902499)
Aah nvm, `IsDust` works on TxOuts.
💬 dergoegge commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459568882)
> is that on master
Sorry I should have clarified, it is on master.
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459568882)
> is that on master
Sorry I should have clarified, it is on master.
💬 fanquake commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2459572965)
Guix Build:
```bash
ea6e6faf76005701a3251872e64ee1dd5c7e1222d28b38a273d9c1851470e927 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/SHA256SUMS.part
b71cc1acf87a5394334149a2cd724e6acd97b2660d7f74d32b97e62f83a539d6 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu-debug.tar.gz
d4879760d76da7aa4a41fe56981a0c2992032ea41e9460ef60036dc57599e9e7 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu.tar.gz
2cbd20b0b10f97fb
...
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2459572965)
Guix Build:
```bash
ea6e6faf76005701a3251872e64ee1dd5c7e1222d28b38a273d9c1851470e927 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/SHA256SUMS.part
b71cc1acf87a5394334149a2cd724e6acd97b2660d7f74d32b97e62f83a539d6 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu-debug.tar.gz
d4879760d76da7aa4a41fe56981a0c2992032ea41e9460ef60036dc57599e9e7 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu.tar.gz
2cbd20b0b10f97fb
...