💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1384972762)
> I'm wondering, does it even make sense to keep such flag at this point? What it would take to adjust all code to going back-and-forth w.r.t. superstale?
I think that would be an oxymoron. We would be adding code to put the node back into IBD state, because of a stale tip check, when the node is updating the tip.
The `fInitialDownload` flag usage is just a speedup. We know for sure that when the flag is true, it is because the node is in IBD. So we don't need to check anything else in suc
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1384972762)
> I'm wondering, does it even make sense to keep such flag at this point? What it would take to adjust all code to going back-and-forth w.r.t. superstale?
I think that would be an oxymoron. We would be adding code to put the node back into IBD state, because of a stale tip check, when the node is updating the tip.
The `fInitialDownload` flag usage is just a speedup. We know for sure that when the flag is true, it is because the node is in IBD. So we don't need to check anything else in suc
...
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1384991649)
> The word `initial` here is indeed confusing... Thinking about something happening only at node bootstrap, which is a more obvious version i think. Can we find a different word now that you're changing it to certainly not mean that?
Hmm, first names coming to my mind are `m_close_to_synced` or `m_close_to_tip`? or, could use the existing term `m_can_direct_fetch`?
Still, maybe better to leave it for a follow-up because, with the back-and-forth functionality, we could also use this flag in
...
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1384991649)
> The word `initial` here is indeed confusing... Thinking about something happening only at node bootstrap, which is a more obvious version i think. Can we find a different word now that you're changing it to certainly not mean that?
Hmm, first names coming to my mind are `m_close_to_synced` or `m_close_to_tip`? or, could use the existing term `m_can_direct_fetch`?
Still, maybe better to leave it for a follow-up because, with the back-and-forth functionality, we could also use this flag in
...
📝 glozow opened a pull request: "rpc: permit any ancestor package through submitpackage"
(https://github.com/bitcoin/bitcoin/pull/28813)
After #26711, validation should be able to handle any ancestor package properly. We can remove the child-with-unconfirmed-parents restriction and the `IsTree` restriction.
In draft while #26711 is still open.
(https://github.com/bitcoin/bitcoin/pull/28813)
After #26711, validation should be able to handle any ancestor package properly. We can remove the child-with-unconfirmed-parents restriction and the `IsTree` restriction.
In draft while #26711 is still open.
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1798634990)
> The "linearize using fees" commit message seems to have an extra line at the end left over from a rebase? The algorithm description seems like it would be better as a code comment rather than a commit message.
Added the algo description to the documentation of the `AcceptPackage` function
> - linearisation + miniminer + unit/fuzz test and bench
> - acceptpackage loop + subpackage linearization + unit/fuzz tests?
I'd prefer to keep the `AncestorPackage` changes (along with its unit/be
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1798634990)
> The "linearize using fees" commit message seems to have an extra line at the end left over from a rebase? The algorithm description seems like it would be better as a code comment rather than a commit message.
Added the algo description to the documentation of the `AcceptPackage` function
> - linearisation + miniminer + unit/fuzz test and bench
> - acceptpackage loop + subpackage linearization + unit/fuzz tests?
I'd prefer to keep the `AncestorPackage` changes (along with its unit/be
...
👍 willcl-ark approved a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1716369031)
tACK 0c5f8c36c90fab4fb39ce39d1b4c204a2edd25c4
Left a few nits which IMO don't need addressing. Tested out new socket type for proxy and onion and found it working as expected.
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1716369031)
tACK 0c5f8c36c90fab4fb39ce39d1b4c204a2edd25c4
Left a few nits which IMO don't need addressing. Tested out new socket type for proxy and onion and found it working as expected.
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1384082654)
nit: in c079e29c266a413de1859c0cbc8ebb55ea6a3203
I get a compilation failure:
```log
net.cpp:481:109: error: no member named 'ToString' in 'Proxy'
LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.ToString(), addrConnect.ToStringAddr(), addrConnect.GetPort());
~~~~~ ^
./logging.h:257:45: note: expanded from macro 'LogPrintLevel'
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1384082654)
nit: in c079e29c266a413de1859c0cbc8ebb55ea6a3203
I get a compilation failure:
```log
net.cpp:481:109: error: no member named 'ToString' in 'Proxy'
LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.ToString(), addrConnect.ToStringAddr(), addrConnect.GetPort());
~~~~~ ^
./logging.h:257:45: note: expanded from macro 'LogPrintLevel'
...
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1384854138)
In c079e29
`ConnectDirectly` and `ConnectThroughProxy` now differ in their primary args, with Directly taking a `CService` and Proxy remaining with `std::string&` (for host and port) which results in:
_src/net.cpp_
```cpp
} else if (use_proxy) {
LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.proxy.ToStringAddrPort(), addrConnect.ToStringAddr(), addrConnect.GetPort());
sock = ConnectThroughProxy(proxy, ad
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1384854138)
In c079e29
`ConnectDirectly` and `ConnectThroughProxy` now differ in their primary args, with Directly taking a `CService` and Proxy remaining with `std::string&` (for host and port) which results in:
_src/net.cpp_
```cpp
} else if (use_proxy) {
LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.proxy.ToStringAddrPort(), addrConnect.ToStringAddr(), addrConnect.GetPort());
sock = ConnectThroughProxy(proxy, ad
...
💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1385000534)
This is an `Assume` now
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1385000534)
This is an `Assume` now
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1385001110)
Hmm, naming is hard.
As the function is called `TipMayBeStale()`, maybe calling it `SyncState` could also work. Thoughts? Or do you have anything else in mind?
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1385001110)
Hmm, naming is hard.
As the function is called `TipMayBeStale()`, maybe calling it `SyncState` could also work. Thoughts? Or do you have anything else in mind?
📝 fanquake opened a pull request: "sanitizer: symbolizer improvements"
(https://github.com/bitcoin/bitcoin/pull/28814)
Explicitly specifying a symbolizer path has fixed the two issues outlined in https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1789703076. It's not completely clear to my why this needs to be explicitly specified in some environments, and not in others, while at the same time `llvm-symbolizer` is already in PATH, however, this has fixed the 2 issues (Fedora & Tumbleweed). My guess is something to do with symlinking, and tooling expecting unversioned/exactly named binaries?
In future,
...
(https://github.com/bitcoin/bitcoin/pull/28814)
Explicitly specifying a symbolizer path has fixed the two issues outlined in https://github.com/bitcoin/bitcoin/pull/28147#issuecomment-1789703076. It's not completely clear to my why this needs to be explicitly specified in some environments, and not in others, while at the same time `llvm-symbolizer` is already in PATH, however, this has fixed the 2 issues (Fedora & Tumbleweed). My guess is something to do with symlinking, and tooling expecting unversioned/exactly named binaries?
In future,
...
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1385015371)
I think before this was to fix a bug, but I fixed that bug in a different way. Regardless, doesn't it make more sense to run `transactionRemovedFromMempool` before syncing the transaction as confirmed?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1385015371)
I think before this was to fix a bug, but I fixed that bug in a different way. Regardless, doesn't it make more sense to run `transactionRemovedFromMempool` before syncing the transaction as confirmed?
💬 glozow commented on pull request "refactor: Miniminer package linearization followups":
(https://github.com/bitcoin/bitcoin/pull/28808#issuecomment-1798706638)
This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp
@@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
+ miniminer_info.emplace_back(grandparent_zero_fee, /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);
@@ -696 +695 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
+
...
(https://github.com/bitcoin/bitcoin/pull/28808#issuecomment-1798706638)
This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp
@@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
+ miniminer_info.emplace_back(grandparent_zero_fee, /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);
@@ -696 +695 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
+
...
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1798760647)
Updated per @andrewtoth feedback.
Improved a test comment for better clarity. Tiny [diff](https://github.com/bitcoin/bitcoin/compare/5628ac55be42c5450c6817ba8dcfe463ceda32a9..60a487dd2430145115fcd0215472a5a2ef9bb090).
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1798760647)
Updated per @andrewtoth feedback.
Improved a test comment for better clarity. Tiny [diff](https://github.com/bitcoin/bitcoin/compare/5628ac55be42c5450c6817ba8dcfe463ceda32a9..60a487dd2430145115fcd0215472a5a2ef9bb090).
🤔 furszy reviewed a pull request: "wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring"
(https://github.com/bitcoin/bitcoin/pull/28546#pullrequestreview-1717909906)
Code review ACK f06016d
Have to admit that I'm still not sure about the first commit, but it isn't blocking if others are happy with it.
(https://github.com/bitcoin/bitcoin/pull/28546#pullrequestreview-1717909906)
Code review ACK f06016d
Have to admit that I'm still not sure about the first commit, but it isn't blocking if others are happy with it.
💬 kevkevinpal commented on pull request "refactor: Miniminer package linearization followups":
(https://github.com/bitcoin/bitcoin/pull/28808#issuecomment-1798778274)
> This diff appears to have added new lines with trailing whitespace. The following changes were suspected:
>
> diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp @@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
>
> * ```
> miniminer_info.emplace_back(grandparent_zero_fee, /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);
> ```
>
>
> @@ -696 +695 @@ BOOST_FIXTURE
...
(https://github.com/bitcoin/bitcoin/pull/28808#issuecomment-1798778274)
> This diff appears to have added new lines with trailing whitespace. The following changes were suspected:
>
> diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp @@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)
>
> * ```
> miniminer_info.emplace_back(grandparent_zero_fee, /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);
> ```
>
>
> @@ -696 +695 @@ BOOST_FIXTURE
...
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1798781365)
Bloom filter flame graph:
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1798781365)
Bloom filter flame graph:
💬 mzumsande commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1798836337)
> I presume this will fix itself, once and if v2 is enabled by default?
After enabling v2 by default for the functional tests, we'd probably still want to run some or all of the tests with `-v2transport=0` to avoid regressions - so I don't think that would change much.
(https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1798836337)
> I presume this will fix itself, once and if v2 is enabled by default?
After enabling v2 by default for the functional tests, we'd probably still want to run some or all of the tests with `-v2transport=0` to avoid regressions - so I don't think that would change much.
👍 maflcko approved a pull request: "sanitizer: symbolizer improvements"
(https://github.com/bitcoin/bitcoin/pull/28814#pullrequestreview-1717966778)
nice, lgtm
(https://github.com/bitcoin/bitcoin/pull/28814#pullrequestreview-1717966778)
nice, lgtm
💬 maflcko commented on pull request "sanitizer: symbolizer improvements":
(https://github.com/bitcoin/bitcoin/pull/28814#discussion_r1385087260)
Is this true on all platforms? If not, what would happen?
(https://github.com/bitcoin/bitcoin/pull/28814#discussion_r1385087260)
Is this true on all platforms? If not, what would happen?
💬 maflcko commented on pull request "sanitizer: symbolizer improvements":
(https://github.com/bitcoin/bitcoin/pull/28814#discussion_r1385090297)
Looks like the CI provides the answer already:
```
==18180==WARNING: invalid path to external symbolizer!
==18180==WARNING: Failed to use and restart external symbolizer!
test/fuzz/FuzzedDataProvider.h:212:47: runtime error: unsigned integer overflow: 9223372036854775807 - 9223372036854775808 cannot be represented in type 'uint64_t' (aka 'unsigned long')
#0 0x556fc34a1b1b (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a12b1b) (BuildId: 3ec865ec
...
(https://github.com/bitcoin/bitcoin/pull/28814#discussion_r1385090297)
Looks like the CI provides the answer already:
```
==18180==WARNING: invalid path to external symbolizer!
==18180==WARNING: Failed to use and restart external symbolizer!
test/fuzz/FuzzedDataProvider.h:212:47: runtime error: unsigned integer overflow: 9223372036854775807 - 9223372036854775808 cannot be represented in type 'uint64_t' (aka 'unsigned long')
#0 0x556fc34a1b1b (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a12b1b) (BuildId: 3ec865ec
...