💬 laanwj commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820458290)
Maybe add something like "if the IDE has an option for this" to make it slightly more clear, out of context, that it's an IDE option, and not something to set with say, `debugedit`.
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820458290)
Maybe add something like "if the IDE has an option for this" to make it slightly more clear, out of context, that it's an IDE option, and not something to set with say, `debugedit`.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)
Interesting. So, for me, the first commit (with a new test) takes forever to execute.
More specifically, the execution hangs on `best_peers.resize(targets_size);`, with `targets_size` being a huge number.
I guess your system handles this gracefully instead?
A clear failure could be triggered by e.g. `Assume(targets_size <= m_states.size());`
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)
Interesting. So, for me, the first commit (with a new test) takes forever to execute.
More specifically, the execution hangs on `best_peers.resize(targets_size);`, with `targets_size` being a huge number.
I guess your system handles this gracefully instead?
A clear failure could be triggered by e.g. `Assume(targets_size <= m_states.size());`
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443766239)
> throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues/17501)
The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443766239)
> throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues/17501)
The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?
💬 fanquake commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2443772429)
Generally I would prefer if we weren't making source code changes to fix (currently edge-case) Windows linking errors; especially if there are more complete fixes we could be making (even if later-on).
It'd be good if all commits explained the problem, and fix. i.e the first commit 4a1accd1df31cba778167b6a3ff9783ffedb46ab `refactor: Inline cs_main definition` has no explanation of what the problem/fix is, and doesn't mention that the change is only needed to fix a build issue when using MSVC,
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2443772429)
Generally I would prefer if we weren't making source code changes to fix (currently edge-case) Windows linking errors; especially if there are more complete fixes we could be making (even if later-on).
It'd be good if all commits explained the problem, and fix. i.e the first commit 4a1accd1df31cba778167b6a3ff9783ffedb46ab `refactor: Inline cs_main definition` has no explanation of what the problem/fix is, and doesn't mention that the change is only needed to fix a build issue when using MSVC,
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820487554)
4234f5ebe133b949080aaa56da8cbdd18b650ff2
Originally, the code forced me to make this call for every peer, that's why i needed it to be deterministic. It's not the case anymore. Do we still need determinism then?
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820487554)
4234f5ebe133b949080aaa56da8cbdd18b650ff2
Originally, the code forced me to make this call for every peer, that's why i needed it to be deterministic. It's not the case anymore. Do we still need determinism then?
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820490053)
Otherwise we can drop `m_deterministic_randomizer` altogether and use some cheap `FastRandomContext` instead.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820490053)
Otherwise we can drop `m_deterministic_randomizer` altogether and use some cheap `FastRandomContext` instead.
💬 itornaza commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2443779806)
Hey @Christewart! Do you plan to update for cmake?
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2443779806)
Hey @Christewart! Do you plan to update for cmake?
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820518288)
There is no need to cast it to double anymore.
What we should do is something like
```
size_t outbounds_target_size = 0;
if (OUTBOUND_FANOUT_DESTINATIONS > outbounds_fanout_tx_relay) outbounds_target_size = OUTBOUND_FANOUT_DESTINATIONS - outbounds_fanout_tx_relay;
```
Perhaps there is a less ugly way to do this. But casting to double seems worse.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820518288)
There is no need to cast it to double anymore.
What we should do is something like
```
size_t outbounds_target_size = 0;
if (OUTBOUND_FANOUT_DESTINATIONS > outbounds_fanout_tx_relay) outbounds_target_size = OUTBOUND_FANOUT_DESTINATIONS - outbounds_fanout_tx_relay;
```
Perhaps there is a less ugly way to do this. But casting to double seems worse.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820521627)
feel free to resolve this and let's talk in the new commit.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820521627)
feel free to resolve this and let's talk in the new commit.
💬 jarolrod commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443813498)
> > throw away work that has previously been a useful metric in issues (#17501)
>
> The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?
I said it's a useful metric, as in given that context, would it not be nice to have the benchmark?
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443813498)
> > throw away work that has previously been a useful metric in issues (#17501)
>
> The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?
I said it's a useful metric, as in given that context, would it not be nice to have the benchmark?
💬 laanwj commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2443815646)
Yes, it's deprecated, but IIRC the best compromise for the supported windows version. After #31172 we can switch to a whole slew of more modern APIs for windows.
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2443815646)
Yes, it's deprecated, but IIRC the best compromise for the supported windows version. After #31172 we can switch to a whole slew of more modern APIs for windows.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820533885)
The following test would break the code if we forget the double cast (discussion [here](https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)). I suggest adding it to the tests.
```
std::tie(in_fanout_targets, out_fanout_targets) = tracker.GetFanoutTargets(Wtxid::FromUint256(frc.rand256()), /*inbounds_fanout_tx_relay=*/0, /*outbounds_fanout_tx_relay=*/100);
BOOST_CHECK(!tracker.ShouldFanoutTo(peer_id0, in_fanout_targets, out_fanout_targets));
```
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820533885)
The following test would break the code if we forget the double cast (discussion [here](https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)). I suggest adding it to the tests.
```
std::tie(in_fanout_targets, out_fanout_targets) = tracker.GetFanoutTargets(Wtxid::FromUint256(frc.rand256()), /*inbounds_fanout_tx_relay=*/0, /*outbounds_fanout_tx_relay=*/100);
BOOST_CHECK(!tracker.ShouldFanoutTo(peer_id0, in_fanout_targets, out_fanout_targets));
```
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443832515)
> as in given that context
I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.
practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443832515)
> as in given that context
I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.
practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820538647)
(technically the code you have is correct, it's just confusing for no reason i think)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820538647)
(technically the code you have is correct, it's just confusing for no reason i think)
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820542894)
4234f5ebe133b949080aaa56da8cbdd18b650ff2
We can just merge the two vectors, no? There is no use for this separation.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820542894)
4234f5ebe133b949080aaa56da8cbdd18b650ff2
We can just merge the two vectors, no? There is no use for this separation.
💬 jarolrod commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443840863)
> > as in given that context
>
> I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.
>
> practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).
I'm not intending to argue, and as i said your main point in the PR is valid.
If a bench didn't exist at
...
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443840863)
> > as in given that context
>
> I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.
>
> practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).
I'm not intending to argue, and as i said your main point in the PR is valid.
If a bench didn't exist at
...
👍 maflcko approved a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2401315598)
Nice. This looks less scary than expected. Left a nit to add more compile time checks, but this looks good either way.
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2401315598)
Nice. This looks less scary than expected. Left a nit to add more compile time checks, but this looks good either way.
💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820457426)
nit in 97dd5fe5128592332c83998825bbeda063815120: in the commit message: Missing `needed *in the* next commit`.
Also, it would be good to add some failing test cases for this stuff.
For example, `"%1"` should fail, because `terminate called after throwing an instance of 'tinyformat::format_error'
what(): tinyformat: Conversion spec incorrectly terminated by end of string`. See https://godbolt.org/z/1eehh1oTv . Also see 51c56e8b38033b96fb3930c2bcba3add2047d324
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820457426)
nit in 97dd5fe5128592332c83998825bbeda063815120: in the commit message: Missing `needed *in the* next commit`.
Also, it would be good to add some failing test cases for this stuff.
For example, `"%1"` should fail, because `terminate called after throwing an instance of 'tinyformat::format_error'
what(): tinyformat: Conversion spec incorrectly terminated by end of string`. See https://godbolt.org/z/1eehh1oTv . Also see 51c56e8b38033b96fb3930c2bcba3add2047d324
💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820504400)
Also, some more "fancy" passing test cases:
* `PassFmt<3>("'%1$- 0+*3$.*2$f'");` (space and `0` are ignored, confusing, but valid)
* `PassFmt<3>("'%- 0+*.*f'");` (same, without pos specs)
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820504400)
Also, some more "fancy" passing test cases:
* `PassFmt<3>("'%1$- 0+*3$.*2$f'");` (space and `0` are ignored, confusing, but valid)
* `PassFmt<3>("'%- 0+*.*f'");` (same, without pos specs)
💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820461851)
Other failing test cases to possibly add: `"%*"`, `"%+*"`, `"%.*"`, ...
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820461851)
Other failing test cases to possibly add: `"%*"`, `"%+*"`, `"%.*"`, ...
💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820543443)
Nit in 1d16d6e6bac994fed7c695f530b9984edcd290bd: I don't really see why this is useful. Maybe I am missing something, but everything compiles by just moving the two members to the derived struct:
```diff
diff --git a/src/tinyformat.h b/src/tinyformat.h
index 56c25d8f7f..6227cdeca0 100644
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -179,21 +179,16 @@ namespace tfm = tinyformat;
namespace tinyformat {
-// Added for Bitcoin Core. Wrapper type for format strings to allow comp
...
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820543443)
Nit in 1d16d6e6bac994fed7c695f530b9984edcd290bd: I don't really see why this is useful. Maybe I am missing something, but everything compiles by just moving the two members to the derived struct:
```diff
diff --git a/src/tinyformat.h b/src/tinyformat.h
index 56c25d8f7f..6227cdeca0 100644
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -179,21 +179,16 @@ namespace tfm = tinyformat;
namespace tinyformat {
-// Added for Bitcoin Core. Wrapper type for format strings to allow comp
...