💬 darosior commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1849887372)
This shouldn't have been closed i think @fanquake.
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1849887372)
This shouldn't have been closed i think @fanquake.
📝 fanquake reopened a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960)
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
---
This PR is part of the [libbitcoinke
...
(https://github.com/bitcoin/bitcoin/pull/28960)
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
---
This PR is part of the [libbitcoinke
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1849912410)
Rebased and added a simple functional test, which includes a (hopefully) deterministic tweak.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1849912410)
Rebased and added a simple functional test, which includes a (hopefully) deterministic tweak.
💬 dergoegge commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1849934451)
Nice find!
I am wondering if this is worth it at this point, since we only fuzz with llvm based compilers. If we had infrastructure that was fuzzing with gcc (e.g. compiling with afl++'s `afl-g++-fast`) then this would clearly make sense. I think if we are going to care about this, we should also add a gcc fuzz CI job and have a linter (maybe a custom clang-tidy plugin?) that checks for dependence on evaluation order (which would probably be a good idea in any case).
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1849934451)
Nice find!
I am wondering if this is worth it at this point, since we only fuzz with llvm based compilers. If we had infrastructure that was fuzzing with gcc (e.g. compiling with afl++'s `afl-g++-fast`) then this would clearly make sense. I think if we are going to care about this, we should also add a gcc fuzz CI job and have a linter (maybe a custom clang-tidy plugin?) that checks for dependence on evaluation order (which would probably be a good idea in any case).
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422369475)
I think just direct parents should be fine - presumably any further generations should have the same version anyway.
I'll continue using `m_ancestors` here since it's what we have access to, but will leave a comment about this.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422369475)
I think just direct parents should be fine - presumably any further generations should have the same version anyway.
I'll continue using `m_ancestors` here since it's what we have access to, but will leave a comment about this.
👍 brunoerg approved a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1775003092)
ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1775003092)
ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5
🚀 fanquake merged a pull request: "fuzz: Improve fuzzing stability for txorphan harness"
(https://github.com/bitcoin/bitcoin/pull/29031)
(https://github.com/bitcoin/bitcoin/pull/29031)
💬 kallewoof commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-1849995402)
This looks reasonable to me, although I'm not super familiar with all the code being changed.
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-1849995402)
This looks reasonable to me, although I'm not super familiar with all the code being changed.
💬 dergoegge commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1849996651)
> Hmm, if I switch to this PR and try compiling, I get bunches of linker errors about multiple definitions, eg
Yea weird, it compiles for me with `afl-clang-lto` but not with regular clang.
> Dropping from 14GB to 3.6GB seems worthwhile for oss-fuzz (and similar), but not worthwhile otherwise to me
I listed more benefits in https://github.com/bitcoin/bitcoin/issues/28971. It is not gonna have a noticeable effect for contributors that aren't regularly fuzzing.
> to get a list of fuzz
...
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1849996651)
> Hmm, if I switch to this PR and try compiling, I get bunches of linker errors about multiple definitions, eg
Yea weird, it compiles for me with `afl-clang-lto` but not with regular clang.
> Dropping from 14GB to 3.6GB seems worthwhile for oss-fuzz (and similar), but not worthwhile otherwise to me
I listed more benefits in https://github.com/bitcoin/bitcoin/issues/28971. It is not gonna have a noticeable effect for contributors that aren't regularly fuzzing.
> to get a list of fuzz
...
👍 stickies-v approved a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1775071503)
re-ACK f761f0306b092c06cecd239060b36cd0ba45fa2c, very nice
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1775071503)
re-ACK f761f0306b092c06cecd239060b36cd0ba45fa2c, very nice
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1850053280)
> @josibake can you add the following helper functions?
>
> * [ ] `CouldBeSilentPayment(CTransactionRef tx)`: checks that it's not a coinbase and that at least one of the outputs is taproot
> See [Silent payment index (for light wallets and consistency check) #28241 (comment)](https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1331881346)
>
> * [ ] `bool GetSilentPaymentsTweakedPubKeySum(std::vector<CTxIn>, std::map<COutPoint, Coin>, CPubKey& tweaked_key) nodiscard`
...
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1850053280)
> @josibake can you add the following helper functions?
>
> * [ ] `CouldBeSilentPayment(CTransactionRef tx)`: checks that it's not a coinbase and that at least one of the outputs is taproot
> See [Silent payment index (for light wallets and consistency check) #28241 (comment)](https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1331881346)
>
> * [ ] `bool GetSilentPaymentsTweakedPubKeySum(std::vector<CTxIn>, std::map<COutPoint, Coin>, CPubKey& tweaked_key) nodiscard`
...
🤔 glozow reviewed a pull request: "fuzz: Improve fuzzing stability for txorphan harness"
(https://github.com/bitcoin/bitcoin/pull/29031#pullrequestreview-1775144033)
concept ACK 15f5a0d0c8ce6b306cdeba6a4777334b848a76aa
(https://github.com/bitcoin/bitcoin/pull/29031#pullrequestreview-1775144033)
concept ACK 15f5a0d0c8ce6b306cdeba6a4777334b848a76aa
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422485145)
the other variant you're referring to here https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259 should cover that case.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422485145)
the other variant you're referring to here https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259 should cover that case.
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850133512)
The BnB search test was passing before as well, because it was expecting that BnB prefer the two input solution over the one input solution at low feerates, and deducting the fee from the target instead of adding the fee to the inputs caused the two input solution to have a better waste score than the one input solution: because the target was reduced the one input solution had a waste that was higher by one input fee than that of the two input solution.
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850133512)
The BnB search test was passing before as well, because it was expecting that BnB prefer the two input solution over the one input solution at low feerates, and deducting the fee from the target instead of adding the fee to the inputs caused the two input solution to have a better waste score than the one input solution: because the target was reduced the one input solution had a waste that was higher by one input fee than that of the two input solution.
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850163591)
> > Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.
>
> Wasn't the test still passing before, when you were deducting them from the target? If so, I would suggest we take a closer look at this test in #28985 to ensure it's testing the thing it's supposed to be testing.
Yes, it was passing but not for the right reason.
Deducting the inputs fees from the target was the
...
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850163591)
> > Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.
>
> Wasn't the test still passing before, when you were deducting them from the target? If so, I would suggest we take a closer look at this test in #28985 to ensure it's testing the thing it's supposed to be testing.
Yes, it was passing but not for the right reason.
Deducting the inputs fees from the target was the
...
🤔 ismaelsadeeq reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1774659127)
>There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will continue to use this or something similar afterward.
If IIUC the pinning issues that are fixed by this new v3 rules are
1. Rule 3 Absolute fee pinning.
2. Rule 5 pinning pinning vector.
I looked at the proposal of cluster mempool the v3 proposals but fail to understand how the addition of cluster mempool might revert the pinning vectors solved by this new policy rules specifically of
...
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1774659127)
>There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will continue to use this or something similar afterward.
If IIUC the pinning issues that are fixed by this new v3 rules are
1. Rule 3 Absolute fee pinning.
2. Rule 5 pinning pinning vector.
I looked at the proposal of cluster mempool the v3 proposals but fail to understand how the addition of cluster mempool might revert the pinning vectors solved by this new policy rules specifically of
...
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422166975)
why move this to the if statement?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422166975)
why move this to the if statement?
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422494963)
I agree with @mzumsande https://github.com/bitcoin/bitcoin/pull/28948/commits/bb979f4c76f4f4f4357a08674df833a5cc6dcf82#r1414612161.
I think it will generally be better to remove the transaction when it was deprioritized and its fee reduced to <= 0 to clear the ambiguity?
tx2 is evicted for having 0 fees but its when we are trimming mempool that its evicted, it should not be in the mempool the moment it's deprioritized not at a later time.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422494963)
I agree with @mzumsande https://github.com/bitcoin/bitcoin/pull/28948/commits/bb979f4c76f4f4f4357a08674df833a5cc6dcf82#r1414612161.
I think it will generally be better to remove the transaction when it was deprioritized and its fee reduced to <= 0 to clear the ambiguity?
tx2 is evicted for having 0 fees but its when we are trimming mempool that its evicted, it should not be in the mempool the moment it's deprioritized not at a later time.
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422364536)
```suggestion
#include <algorithm>
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422364536)
```suggestion
#include <algorithm>
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422497121)
```cpp
if (ancestors.empty()) {
return std::nullopt;
}
```
should be checked before we check this size?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422497121)
```cpp
if (ancestors.empty()) {
return std::nullopt;
}
```
should be checked before we check this size?
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422429402)
Is it worth adding a comment saying `Package` does not refer to a child and all of its unconfirmed parents? The `Package` in param can contain unrelated transaction.
untill I saw the test, I had the assumption of otherwise.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422429402)
Is it worth adding a comment saying `Package` does not refer to a child and all of its unconfirmed parents? The `Package` in param can contain unrelated transaction.
untill I saw the test, I had the assumption of otherwise.