💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612981132)
Why not write this like?
```
#define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
do {
Assume(level >= BCLog::Level::Info);
if (LogAcceptCategory((category), (level))) {
LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);
}
} while (0)
```
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612981132)
Why not write this like?
```
#define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
do {
Assume(level >= BCLog::Level::Info);
if (LogAcceptCategory((category), (level))) {
LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);
}
} while (0)
```
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3644985274)
This is good LogPrintLevel just leads to more confusion, standardise the macros!
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3644985274)
This is good LogPrintLevel just leads to more confusion, standardise the macros!
💬 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
...