💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825868980)
No idea! Gone now.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825868980)
No idea! Gone now.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869389)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869389)
Fixed.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869528)
Moved.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869528)
Moved.
📝 brunoerg opened a pull request: "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction"
(https://github.com/bitcoin/bitcoin/pull/31203)
This PR limites the value of `m_confirm_target` to avoid `implicit-integer-sign-change`:
```
/ci_container_base/src/wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294967292 (32-bit, unsigned) to type 'int' changed the value to -4 (32-bit, signed)
#0 0x55b6fd26c021 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/fees.cpp:58:58
#
...
(https://github.com/bitcoin/bitcoin/pull/31203)
This PR limites the value of `m_confirm_target` to avoid `implicit-integer-sign-change`:
```
/ci_container_base/src/wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294967292 (32-bit, unsigned) to type 'int' changed the value to -4 (32-bit, signed)
#0 0x55b6fd26c021 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/fees.cpp:58:58
#
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869661)
Reworded.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825869661)
Reworded.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825870218)
Tried to remove all instances of setMemPoolParents and setMemPoolChildren from code comments.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825870218)
Tried to remove all instances of setMemPoolParents and setMemPoolChildren from code comments.
💬 brunoerg commented on pull request "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction":
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2451947424)
cc: @maflcko
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2451947424)
cc: @maflcko
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871071)
This was a good suggestion, thanks -- with the introduction of changesets, we can make `RemoveStaged()` a private method now, and so I updated the commit message to explain that the public removal methods may not be invoked while a changeset is outstanding.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871071)
This was a good suggestion, thanks -- with the introduction of changesets, we can make `RemoveStaged()` a private method now, and so I updated the commit message to explain that the public removal methods may not be invoked while a changeset is outstanding.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871365)
Gone thanks
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871365)
Gone thanks
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871510)
Done
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825871510)
Done
💬 sr-gi commented on pull request "ci: Do not error on unused-member-function in test each commit":
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2451951381)
> Can you add a dummy commit to trigger the CI, so that the flag difference can be seen, and to confirm that there are no unwanted interactions?
Done, will drop + force-push once CI runs
(https://github.com/bitcoin/bitcoin/pull/31187#issuecomment-2451951381)
> Can you add a dummy commit to trigger the CI, so that the flag difference can be seen, and to confirm that there are no unwanted interactions?
Done, will drop + force-push once CI runs
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825876761)
Because if you remove the parent from the reconciliation set but not the children, it could be the case that you reconcile with that peer before fanning out the parent, and all those children will be orphan
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825876761)
Because if you remove the parent from the reconciliation set but not the children, it could be the case that you reconcile with that peer before fanning out the parent, and all those children will be orphan
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825888636)
Because it is doing more than just moving the code around, so I thought it may be easier for reviewers already familiar with the original approach to understand the changes. I wouldn't mind squashing it once all the discussion around `RelayTranaction` is resolved
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825888636)
Because it is doing more than just moving the code around, so I thought it may be easier for reviewers already familiar with the original approach to understand the changes. I wouldn't mind squashing it once all the discussion around `RelayTranaction` is resolved
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825893170)
Happy to consider an alternative approach that allows us to check there are no collisions without having to expose a public method if you have something in mind
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825893170)
Happy to consider an alternative approach that allows us to check there are no collisions without having to expose a public method if you have something in mind
⚠️ pinheadmz opened an issue: "Source code mapping for debugger has changed since cmake"
(https://github.com/bitcoin/bitcoin/issues/31204)
developer-notes.md says:
>1. Configure source file mapping.
>
>For `gdb` create or append to `.gdbinit` file:
>```
>set substitute-path ./src /path/to/project/root/src
>```
>
>For `lldb` create or append to `.lldbinit` file:
>```
>settings set target.source-map ./src /path/to/project/root/src
>```
But I found I needed to create a `.lldbinit` file with these lines:
```
settings append target.source-map build/src/test/ /Users/matthewzipkin/Desktop/work/bitcoin/
settings append
...
(https://github.com/bitcoin/bitcoin/issues/31204)
developer-notes.md says:
>1. Configure source file mapping.
>
>For `gdb` create or append to `.gdbinit` file:
>```
>set substitute-path ./src /path/to/project/root/src
>```
>
>For `lldb` create or append to `.lldbinit` file:
>```
>settings set target.source-map ./src /path/to/project/root/src
>```
But I found I needed to create a `.lldbinit` file with these lines:
```
settings append target.source-map build/src/test/ /Users/matthewzipkin/Desktop/work/bitcoin/
settings append
...
💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451988229)
> this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing.
I don't think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build (with different build options) and expect it to work, or be a supported use-case. This has also been the case up until two weeks ago, before commit 9f243cd7fa6654e3b71ba6bff82cceed547c5d
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451988229)
> this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing.
I don't think it is a supported use-case to take libraries from one build (with different build options) and drop them into another build (with different build options) and expect it to work, or be a supported use-case. This has also been the case up until two weeks ago, before commit 9f243cd7fa6654e3b71ba6bff82cceed547c5d
...
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825901123)
Ok, given new interface seems impossible to hit. Added another test case to `ComapreChunks` to just make sure it was well-defined:
```
diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
index c6d06c937c..d610661878 100644
--- a/src/test/rbf_tests.cpp
+++ b/src/test/rbf_tests.cpp
@@ -631,20 +631,32 @@ BOOST_AUTO_TEST_CASE(feerate_chunks_utilities)
{
// Sanity check the correctness of the feerate chunks comparison.
// A strictly better case.
std::vector<Fee
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825901123)
Ok, given new interface seems impossible to hit. Added another test case to `ComapreChunks` to just make sure it was well-defined:
```
diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
index c6d06c937c..d610661878 100644
--- a/src/test/rbf_tests.cpp
+++ b/src/test/rbf_tests.cpp
@@ -631,20 +631,32 @@ BOOST_AUTO_TEST_CASE(feerate_chunks_utilities)
{
// Sanity check the correctness of the feerate chunks comparison.
// A strictly better case.
std::vector<Fee
...
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825906517)
history is cleaner, thank you
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1825906517)
history is cleaner, thank you
💬 maflcko commented on pull request "fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction":
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2452007044)
lgtm ACK 5a26cf7773efdc316ef8235c541ad39e230cd10a
(https://github.com/bitcoin/bitcoin/pull/31203#issuecomment-2452007044)
lgtm ACK 5a26cf7773efdc316ef8235c541ad39e230cd10a
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825913918)
I don't think this comment is accurate, since this can never be the case. A transaction considered for relay cannot have in-mempool descendants by definition, since they would have been orphans at the time of considering them.
I cannot recall if this is a re-word of the previous approach, which tried to consider descendants, or I tried to refer to the collisions and it did;t make much sense.
I'll remove the comment
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1825913918)
I don't think this comment is accurate, since this can never be the case. A transaction considered for relay cannot have in-mempool descendants by definition, since they would have been orphans at the time of considering them.
I cannot recall if this is a re-word of the previous approach, which tried to consider descendants, or I tried to refer to the collisions and it did;t make much sense.
I'll remove the comment