💬 dergoegge commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459317676)
oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:
```
$ FUZZ=txdownloadman_impl out/libfuzzer/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.
```
[clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt](https://github.com/user-att
...
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459317676)
oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:
```
$ FUZZ=txdownloadman_impl out/libfuzzer/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.
```
[clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt](https://github.com/user-att
...
💬 dergoegge commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2459379945)
Perhaps relevant for the discussion around removing `testBlockValidity`: https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2459379945)
Perhaps relevant for the discussion around removing `testBlockValidity`: https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947
💬 laanwj commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#discussion_r1830802184)
Might want to do some basic checks on the pagesize; eg
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0):
```
(https://github.com/bitcoin/bitcoin/pull/31222#discussion_r1830802184)
Might want to do some basic checks on the pagesize; eg
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0):
```
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459428224)
`99b1f88fe8...cf83f0c14c`: rebase due to conflicts and mock the sleeps
> > I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
> Sounds good to me.
Done. `CThreadInterrupt` can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459428224)
`99b1f88fe8...cf83f0c14c`: rebase due to conflicts and mock the sleeps
> > I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
> Sounds good to me.
Done. `CThreadInterrupt` can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!
💬 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.