💬 stratospher commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3644999563)
planning on picking this up in a separate PR - unless someone already has a branch ready (happy to review in that case!).
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3644999563)
planning on picking this up in a separate PR - unless someone already has a branch ready (happy to review in that case!).
💬 Ataraxia009 commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3645025381)
> if it's needed in the future, it's trivial to reintroduce it.
It seems like a strong enough option to leave as is still but ill defer to others i dont have a strong opinion
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3645025381)
> if it's needed in the future, it's trivial to reintroduce it.
It seems like a strong enough option to leave as is still but ill defer to others i dont have a strong opinion
💬 Chand-ra commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613095907)
`GetInputWeight()` returns a `std::optional<int64_t>`, so I don't think it can return a `nullptr`. It **does** return a `std::nullopt` when the coin is not selected, but I think adding a check for that would be too close to duplicating the implementation.
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613095907)
`GetInputWeight()` returns a `std::optional<int64_t>`, so I don't think it can return a `nullptr`. It **does** return a `std::nullopt` when the coin is not selected, but I think adding a check for that would be too close to duplicating the implementation.
👍 vasild approved a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3570511275)
ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3570511275)
ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d
💬 vasild commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2613108267)
There are about 60k ports to choose from. There must be a way to pick in a collision free manner even without reuse...
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2613108267)
There are about 60k ports to choose from. There must be a way to pick in a collision free manner even without reuse...
💬 Chand-ra commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613109116)
Yes, that is correct and intentional.
`Select()` assigns a default sequential position, but the goal of this test is to exercise the `SetPosition()` setter directly. We want to verify that the `PreselectedInput` object handles arbitrary position values correctly, even if they differ from the default sequence assigned by `Select()`.
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613109116)
Yes, that is correct and intentional.
`Select()` assigns a default sequential position, but the goal of this test is to exercise the `SetPosition()` setter directly. We want to verify that the `PreselectedInput` object handles arbitrary position values correctly, even if they differ from the default sequence assigned by `Select()`.
💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613228151)
> Why not write it this like?
>
> ```
> #define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
> do {
> Assume(level < BCLog::Level::Info);
> if (LogAcceptCategory((category), (level))) {
> LogPrintLevel_(category, level, false, __VA_ARGS__);
> }
> } while (0)
> ```
That'd conceptually re-introduce the CVE
...
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613228151)
> Why not write it this like?
>
> ```
> #define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
> do {
> Assume(level < BCLog::Level::Info);
> if (LogAcceptCategory((category), (level))) {
> LogPrintLevel_(category, level, false, __VA_ARGS__);
> }
> } while (0)
> ```
That'd conceptually re-introduce the CVE
...
💬 maflcko commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613241442)
I've added a new `LogDebug(BCLog::HTTP,` in the line below, to clarify this in the test itself. If that is not sufficient, I am happy to push any diff or commit here, if someone writes one. Also, I am happy to review a separate pull, or a follow-up.
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613241442)
I've added a new `LogDebug(BCLog::HTTP,` in the line below, to clarify this in the test itself. If that is not sufficient, I am happy to push any diff or commit here, if someone writes one. Also, I am happy to review a separate pull, or a follow-up.
👍 vasild approved a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3570749656)
ACK 9f39a8309cb0a23df03937b0b225aff8ce9e1ec6
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3570749656)
ACK 9f39a8309cb0a23df03937b0b225aff8ce9e1ec6
📝 maflcko opened a pull request: "lint: Remove confusing, redundant, and brittle lint-spelling"
(https://github.com/bitcoin/bitcoin/pull/34053)
`codespell` was a fun experiment. However, it has many issues, when used in this project:
* The number of false-positives and true-positives are in the same ballpark. There are also many false-negatives, so the overall net-benefit is questionable.
* There is often confusion around spelling errors leading to a failing CI (they do not, which was intended).
* LLMs released this year are capable to detect typos with less false-positives and less false-negatives, so the `codespell` integration i
...
(https://github.com/bitcoin/bitcoin/pull/34053)
`codespell` was a fun experiment. However, it has many issues, when used in this project:
* The number of false-positives and true-positives are in the same ballpark. There are also many false-negatives, so the overall net-benefit is questionable.
* There is often confusion around spelling errors leading to a failing CI (they do not, which was intended).
* LLMs released this year are capable to detect typos with less false-positives and less false-negatives, so the `codespell` integration i
...
💬 l0rinc commented on pull request "fuzz: Add tests for `CCoinControl` methods":
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613401863)
I meant `nullopt` of course. We should at least exercise that code path.
(https://github.com/bitcoin/bitcoin/pull/34026#discussion_r2613401863)
I meant `nullopt` of course. We should at least exercise that code path.
💬 l0rinc commented on pull request "lint: Remove confusing, redundant, and brittle lint-spelling":
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645520119)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645520119)
ACK fa904fc683c0892eb800ddb986fdc0c646721077
💬 sedited commented on pull request "lint: Remove confusing, redundant, and brittle lint-spelling":
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645523026)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/34053#issuecomment-3645523026)
Concept ACK
📝 sedited opened a pull request: "net processing: Add ibd check before processing block for txdownloadman"
(https://github.com/bitcoin/bitcoin/pull/34054)
Calculating the rolling bloom filters takes some CPU time from the scheduler thread. This can be observed for example in [this flamegraph](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-172/20066462508/mainnet-default-instrumented-base-flamegraph.svg?x=920203898521&y=780), where handling the filter takes about 2.6% of total time.
During ibd the entries in the tx download bloom filter are just continuously rolled over, and aren't consumed, since no mempool is maintained at the time.
...
(https://github.com/bitcoin/bitcoin/pull/34054)
Calculating the rolling bloom filters takes some CPU time from the scheduler thread. This can be observed for example in [this flamegraph](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-172/20066462508/mainnet-default-instrumented-base-flamegraph.svg?x=920203898521&y=780), where handling the filter takes about 2.6% of total time.
During ibd the entries in the tx download bloom filter are just continuously rolled over, and aren't consumed, since no mempool is maintained at the time.
...
🤔 Eunovo reviewed a pull request: "Implementation of SwiftSync"
(https://github.com/bitcoin/bitcoin/pull/34004#pullrequestreview-3570977196)
Since this SwiftSync implementation uses `assumevalid` assumptions, it makes sense to reason about attacks that apply to `assumevalid`, here as well.
What happens if an attack gives a SwiftSync node a malicious hint file and eclipses the node so that it is denied an honest chain? Assumevalid uses:
- the minimumchain work check to ensure that scripts are verified when the node is denied access to "any chain at least as good as the expected chain"
- the equivalent time check to force the attacker
...
(https://github.com/bitcoin/bitcoin/pull/34004#pullrequestreview-3570977196)
Since this SwiftSync implementation uses `assumevalid` assumptions, it makes sense to reason about attacks that apply to `assumevalid`, here as well.
What happens if an attack gives a SwiftSync node a malicious hint file and eclipses the node so that it is denied an honest chain? Assumevalid uses:
- the minimumchain work check to ensure that scripts are verified when the node is denied access to "any chain at least as good as the expected chain"
- the equivalent time check to force the attacker
...
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613617565)
sounds good
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613617565)
sounds good
💬 maflcko commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2613636332)
this was picked in https://github.com/bitcoin/bitcoin/pull/34054, so i guess this can be closed again?
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2613636332)
this was picked in https://github.com/bitcoin/bitcoin/pull/34054, so i guess this can be closed again?
💬 stratospher commented on issue "qa: Assertion in `p2p_v2_misbehaving.py`":
(https://github.com/bitcoin/bitcoin/issues/34035#issuecomment-3645779184)
ok looks like python event loop asynchronous behaviour.
- it's scheduling connection to `peer1` and then scheduling a connection to `peer2`
- but it is actually connecting to `peer2` first (which gets assigned peer id = 1 on c++ side) and only later connecting to the peer we're interested in `peer1` (which gets assigned peer id = 2 on c++ side) and we're seeing a log mismatch
ideally, we let the event loop do whatever it wants and then use the actual peer id in `expected_debug_message` - as in
...
(https://github.com/bitcoin/bitcoin/issues/34035#issuecomment-3645779184)
ok looks like python event loop asynchronous behaviour.
- it's scheduling connection to `peer1` and then scheduling a connection to `peer2`
- but it is actually connecting to `peer2` first (which gets assigned peer id = 1 on c++ side) and only later connecting to the peer we're interested in `peer1` (which gets assigned peer id = 2 on c++ side) and we're seeing a log mismatch
ideally, we let the event loop do whatever it wants and then use the actual peer id in `expected_debug_message` - as in
...
⚠️ Manishdudi700 opened an issue: "Showing error:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt."
(https://github.com/bitcoin/bitcoin/issues/34055)
When i am trying to transfer funds from my bitcoin core wallet it is showing a error which says:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.
It is also showing the same error in console of bitcoin core app after entering passphrase when i tried to dumpwallet
1. Does it me
...
(https://github.com/bitcoin/bitcoin/issues/34055)
When i am trying to transfer funds from my bitcoin core wallet it is showing a error which says:- Error unlocking wallet: some keys decrypt but not all. Your wallet file may be corrupt.
It is also showing the same error in console of bitcoin core app after entering passphrase when i tried to dumpwallet
1. Does it me
...
💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613683807)
> I've added a new `LogDebug(BCLog::HTTP,` in the line below, to clarify this in the test itself.
That just made me wonder "wait why does this pass" rather than make it obvious that this was enabled in `LogSetup`. It's confusing because in normal operation `Info` is the default level. I think the below diff is all that's needed to make this test self-documenting.
<details>
<summary>git diff on fa35682637</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_test
...
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2613683807)
> I've added a new `LogDebug(BCLog::HTTP,` in the line below, to clarify this in the test itself.
That just made me wonder "wait why does this pass" rather than make it obvious that this was enabled in `LogSetup`. It's confusing because in normal operation `Info` is the default level. I think the below diff is all that's needed to make this test self-documenting.
<details>
<summary>git diff on fa35682637</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_test
...
💬 maflcko commented on pull request "net processing: Add ibd check before processing block for txdownloadman":
(https://github.com/bitcoin/bitcoin/pull/34054#issuecomment-3645846544)
> During ibd the entries in the tx download bloom filter are just continuously rolled over, and aren't consumed, since no mempool is maintained at the time.
I think this could be clarified a bit, why this is safe. My understanding is that a mempool does exist and is maintained (albeit it will normally be empty). The mempool will also happily accept transactions via RPC, or the wallet. However, this is fine, because they don't interact with the orphanage and the txdownloadman, because adding a
...
(https://github.com/bitcoin/bitcoin/pull/34054#issuecomment-3645846544)
> During ibd the entries in the tx download bloom filter are just continuously rolled over, and aren't consumed, since no mempool is maintained at the time.
I think this could be clarified a bit, why this is safe. My understanding is that a mempool does exist and is maintained (albeit it will normally be empty). The mempool will also happily accept transactions via RPC, or the wallet. However, this is fine, because they don't interact with the orphanage and the txdownloadman, because adding a
...