💬 brunoerg commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379532585)
> Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.
Sounds good to me as well. Closing this for now.
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379532585)
> Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.
Sounds good to me as well. Closing this for now.
✅ brunoerg closed a pull request: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984)
(https://github.com/bitcoin/bitcoin/pull/30984)
🤔 stickies-v reviewed a pull request: "[28.x] backports and finalize (or rc3)"
(https://github.com/bitcoin/bitcoin/pull/30959#pullrequestreview-2334037773)
code LGTM 98745e03ffc9f69f901b827e19e4d8d645a27112
Verified all backport commits are clean and make sense, and that I'm getting the same manpages. I think `doc/release-notes.md` still needs to be updated though?
(https://github.com/bitcoin/bitcoin/pull/30959#pullrequestreview-2334037773)
code LGTM 98745e03ffc9f69f901b827e19e4d8d645a27112
Verified all backport commits are clean and make sense, and that I'm getting the same manpages. I think `doc/release-notes.md` still needs to be updated though?
⚠️ maflcko opened an issue: "ci: Intermittent issue in feature_fee_estimation.py in sanity_check_rbf_estimates (Block sync timed out after 2400s)"
(https://github.com/bitcoin/bitcoin/issues/30990)
A quick skim of the log looks like there are two blocks at the same height, but I haven't looked too closely yet.
https://cirrus-ci.com/task/6299307458953216?logs=ci#L4251
(https://github.com/bitcoin/bitcoin/issues/30990)
A quick skim of the log looks like there are two blocks at the same height, but I haven't looked too closely yet.
https://cirrus-ci.com/task/6299307458953216?logs=ci#L4251
💬 mzumsande commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2379597880)
Concept ACK
> In #29618 and here, I don't see the motivation to forbid v1 connections.
Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection. You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split. If som
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2379597880)
Concept ACK
> In #29618 and here, I don't see the motivation to forbid v1 connections.
Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection. You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split. If som
...
📝 ismaelsadeeq opened a pull request: "test: enable running independent functional test methods"
(https://github.com/bitcoin/bitcoin/pull/30991)
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but
...
(https://github.com/bitcoin/bitcoin/pull/30991)
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but
...
💬 stickies-v commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1778860008)
Is there a reason to set `check_ratio`? It doesn't seem necessary?
```suggestion
CTxMemPool::Options mempool_opts{};
```
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1778860008)
Is there a reason to set `check_ratio`? It doesn't seem necessary?
```suggestion
CTxMemPool::Options mempool_opts{};
```
🤔 stickies-v reviewed a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334139707)
Strong concept ACK!
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334139707)
Strong concept ACK!
🤔 l0rinc requested changes to a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2333963688)
I have concerns about the translations, using raw format instead of adjusting the validator, and I think we could sneak in a fix for unterminated positionals.
<details>
<summary>Patch</summary>
```patch
Index: src/bitcoin-cli.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
--- a/src/bitcoin-cli.cpp (revision 8845a566
...
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2333963688)
I have concerns about the translations, using raw format instead of adjusting the validator, and I think we could sneak in a fix for unterminated positionals.
<details>
<summary>Patch</summary>
```patch
Index: src/bitcoin-cli.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
--- a/src/bitcoin-cli.cpp (revision 8845a566
...
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778758009)
is this bug still possible now? If it is, consider updating the `strprintf` example
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778758009)
is this bug still possible now? If it is, consider updating the `strprintf` example
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778754066)
nit: we could avoid mentioning compile-time twice:
```suggestion
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.
```
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778754066)
nit: we could avoid mentioning compile-time twice:
```suggestion
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.
```
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778767590)
I'm not sure I understand how this change is supposed to work.
`"Copyright (C) %i-%i"` can be translated by checking the exact string from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoinstrings.cpp#L280, but how is `"Copyright (C) 2009-2024"` be supposed to have a translated counterpart?
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778767590)
I'm not sure I understand how this change is supposed to work.
`"Copyright (C) %i-%i"` can be translated by checking the exact string from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoinstrings.cpp#L280, but how is `"Copyright (C) 2009-2024"` be supposed to have a translated counterpart?
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778855186)
Unrelated:
just realized that we should eventually add `"%1"` passes, since `while ('0' <= *it && *it <= '9') {` can go beyond the string and passed accidentally - but seems to fail in tinyformat, see: https://godbolt.org/z/nbTKnrqch.
Should I add a new PR for that of do you want to add that case here?
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778855186)
Unrelated:
just realized that we should eventually add `"%1"` passes, since `while ('0' <= *it && *it <= '9') {` can go beyond the string and passed accidentally - but seems to fail in tinyformat, see: https://godbolt.org/z/nbTKnrqch.
Should I add a new PR for that of do you want to add that case here?
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778776885)
👍, should have been there all along
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778776885)
👍, should have been there all along
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778768954)
lol
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778768954)
lol
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778783288)
for consistency, consider using strprintf here as well:
```suggestion
BOOST_CHECK_MESSAGE(ret == expected, strprintf("[%s] mismatching requestables", comment");
```
----
Would be cool if we could use `BOOST_CHECK_EQUAL_COLLECTIONS` instead, but may not be trivial to do, haven't investigated in detail.
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778783288)
for consistency, consider using strprintf here as well:
```suggestion
BOOST_CHECK_MESSAGE(ret == expected, strprintf("[%s] mismatching requestables", comment");
```
----
Would be cool if we could use `BOOST_CHECK_EQUAL_COLLECTIONS` instead, but may not be trivial to do, haven't investigated in detail.
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882137)
nit:
```suggestion
// Ensure invalid format strings don't throw at run-time
```
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882137)
nit:
```suggestion
// Ensure invalid format strings don't throw at run-time
```
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882653)
We could add the examples from `bitcoin-cli` here:
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_SpecificUsageTests)
{
// These format strings contain '*' and should skip validation
PassFmt<6>("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ");
PassFmt<5>("%*s %-*s%s\n");
PassFmt<22>(
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n");
PassFmt<2>(" ms ms sec sec min min %*
...
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882653)
We could add the examples from `bitcoin-cli` here:
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_SpecificUsageTests)
{
// These format strings contain '*' and should skip validation
PassFmt<6>("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ");
PassFmt<5>("%*s %-*s%s\n");
PassFmt<22>(
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n");
PassFmt<2>(" ms ms sec sec min min %*
...
💬 0xB10C commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2379684081)
Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/fb8142e7eb7f98f3. Produces something like this based on the size and from-peers (color) of the orphans.

(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2379684081)
Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/fb8142e7eb7f98f3. Produces something like this based on the size and from-peers (color) of the orphans.

💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716)
To avoid a race, this needs to lock `kernel_notifications.m_tip_block_mutex` before calling notify. `(*node.shutdown_signal)()` doesn't necessarily need to happen under the lock, but a lock is required after the signal in order to synchronize. Otherwise the update could be missed and we could potentially wait forever.
This is currently an example of [this trap](https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/) (see the "An atomic predicate"
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716)
To avoid a race, this needs to lock `kernel_notifications.m_tip_block_mutex` before calling notify. `(*node.shutdown_signal)()` doesn't necessarily need to happen under the lock, but a lock is required after the signal in order to synchronize. Otherwise the update could be missed and we could potentially wait forever.
This is currently an example of [this trap](https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/) (see the "An atomic predicate"
...