💬 maflcko commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158089097)
It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158089097)
It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2158091611)
The false positive CI error still happens. I am taking another look.
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2158091611)
The false positive CI error still happens. I am taking another look.
💬 hebasto commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158131886)
> It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
Done.
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158131886)
> It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e
Done.
💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1633124579)
Done.
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1633124579)
Done.
💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2158146838)
Made a few small changes still:
```diff
@@ -107,7 +107,11 @@ class IntBitSet
return *this;
}
/** Get the current bit position (only if != IteratorEnd). */
- constexpr const unsigned& operator*() const noexcept { return m_pos; }
+ constexpr unsigned operator*() const noexcept
+ {
+ Assume(m_val != 0);
+ return m_pos;
+ }
};
public:
@@ -231,6 +235,8 @@ class MultiIntBitSet
static constex
...
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2158146838)
Made a few small changes still:
```diff
@@ -107,7 +107,11 @@ class IntBitSet
return *this;
}
/** Get the current bit position (only if != IteratorEnd). */
- constexpr const unsigned& operator*() const noexcept { return m_pos; }
+ constexpr unsigned operator*() const noexcept
+ {
+ Assume(m_val != 0);
+ return m_pos;
+ }
};
public:
@@ -231,6 +235,8 @@ class MultiIntBitSet
static constex
...
🤔 shaavan reviewed a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2107588105)
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 💯
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2107588105)
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 💯
👋 fanquake's pull request is ready for review: "[27.1] Finalize"
(https://github.com/bitcoin/bitcoin/pull/30222)
(https://github.com/bitcoin/bitcoin/pull/30222)
👋 fanquake's pull request is ready for review: "depends: remove `FORCE_USE_SYSTEM_CLANG`"
(https://github.com/bitcoin/bitcoin/pull/30201)
(https://github.com/bitcoin/bitcoin/pull/30201)
💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1633146879)
I guess I am confused with the terms "active" and "background" chain. I assume the active chain is the snapshot chain which starts at height `299`, and the divergent chain is the one stuck at `298`. I base this on the fact that when I run `getbestblockhash` after `loadtxoutset`, I get the hash of the snapshot tip. However, I might be mistaken. Shouldn't the divergent chain be rewound to `START_HEIGHT` and become the background validation chain?
To address your questions:
1. The divergent c
...
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1633146879)
I guess I am confused with the terms "active" and "background" chain. I assume the active chain is the snapshot chain which starts at height `299`, and the divergent chain is the one stuck at `298`. I base this on the fact that when I run `getbestblockhash` after `loadtxoutset`, I get the hash of the snapshot tip. However, I might be mistaken. Shouldn't the divergent chain be rewound to `START_HEIGHT` and become the background validation chain?
To address your questions:
1. The divergent c
...
💬 brunoerg commented on issue "fuzz: crypter: Abrt in __cxxabiv1::failed_throw":
(https://github.com/bitcoin/bitcoin/issues/30251#issuecomment-2158194986)
Maybe it's not good to use `ConsumeRandomLengthByteVector`, worthes limiting its size.
https://github.com/bitcoin/bitcoin/blob/cad127235e307d7de0e9cc04708dbd31aa6c24b0/src/wallet/test/fuzz/crypter.cpp#L59
(https://github.com/bitcoin/bitcoin/issues/30251#issuecomment-2158194986)
Maybe it's not good to use `ConsumeRandomLengthByteVector`, worthes limiting its size.
https://github.com/bitcoin/bitcoin/blob/cad127235e307d7de0e9cc04708dbd31aa6c24b0/src/wallet/test/fuzz/crypter.cpp#L59
💬 fanquake commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2158204184)
Added a commit to rename some references to macho ld64 to lld.
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2158204184)
Added a commit to rename some references to macho ld64 to lld.
💬 brunoerg commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2158209801)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2158209801)
Concept ACK
💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633197400)
> We could make it so that a peek read only returns one byte and a subsequent normal read return N bytes where the first byte corresponds to the peek byte.
This was the case before this PR, right?
> But I'm not sure if that would change much for our use case.
Where can I see that use case? Is there some code that does not work without the changes to `FuzzedSock::Recv()` done in this PR?
The doc you cited, in my understanding, means that what I described in the first comment of this t
...
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633197400)
> We could make it so that a peek read only returns one byte and a subsequent normal read return N bytes where the first byte corresponds to the peek byte.
This was the case before this PR, right?
> But I'm not sure if that would change much for our use case.
Where can I see that use case? Is there some code that does not work without the changes to `FuzzedSock::Recv()` done in this PR?
The doc you cited, in my understanding, means that what I described in the first comment of this t
...
🤔 furszy reviewed a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2107711418)
utACK 0000276b31ce
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2107711418)
utACK 0000276b31ce
👍 BrandonOdiwuor approved a pull request: "log: use error level for critical log messages"
(https://github.com/bitcoin/bitcoin/pull/30255#pullrequestreview-2107735380)
ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65 - Replacing LogPrintf with Logerror to enhance critical error logging
(https://github.com/bitcoin/bitcoin/pull/30255#pullrequestreview-2107735380)
ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65 - Replacing LogPrintf with Logerror to enhance critical error logging
👍 ryanofsky approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2107726655)
Code review ACK 09ef322acc0a88a9e119f74923399598984c68f6. Just fuzz test error checking fix and updated comment since last review
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2107726655)
Code review ACK 09ef322acc0a88a9e119f74923399598984c68f6. Just fuzz test error checking fix and updated comment since last review
💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1633207579)
In commit "[[refactor]] Check CTxMemPool options in constructor" (09ef322acc0a88a9e119f74923399598984c68f6)
It's a shame this has to return a pointer now. It would have been nice to avoid the unreachable code with something like [scope_exit](https://en.cppreference.com/w/cpp/experimental/scope_exit), but I guess that is still experimental and hasn't been standardized.
```c++
bilingual_str error;
auto error_check{std::experimental::scope_exit([&] { Assert(error.empty()); })};
...
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1633207579)
In commit "[[refactor]] Check CTxMemPool options in constructor" (09ef322acc0a88a9e119f74923399598984c68f6)
It's a shame this has to return a pointer now. It would have been nice to avoid the unreachable code with something like [scope_exit](https://en.cppreference.com/w/cpp/experimental/scope_exit), but I guess that is still experimental and hasn't been standardized.
```c++
bilingual_str error;
auto error_check{std::experimental::scope_exit([&] { Assert(error.empty()); })};
...
🤔 vasild reviewed a pull request: "fuzz: Make FuzzedSock fuzz friendlier"
(https://github.com/bitcoin/bitcoin/pull/30211#pullrequestreview-2107713174)
I think this was merged prematurely.
(https://github.com/bitcoin/bitcoin/pull/30211#pullrequestreview-2107713174)
I think this was merged prematurely.
💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366)
Now it will not overflow `buf` but it will lose the trailing `N - M` bytes from the peek.
To fix this, when assigning `random_bytes` earlier, it should only take/pop the first `len` bytes from `m_peek_data`.
I think all this may be unnecessary if the app has to work with peek only returning 1 byte, as discussed in https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330207
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366)
Now it will not overflow `buf` but it will lose the trailing `N - M` bytes from the peek.
To fix this, when assigning `random_bytes` earlier, it should only take/pop the first `len` bytes from `m_peek_data`.
I think all this may be unnecessary if the app has to work with peek only returning 1 byte, as discussed in https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1623330207
💬 vasild commented on pull request "fuzz: Make FuzzedSock fuzz friendlier":
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919)
`std::vector<uint8_t>` can be used instead of `std::optional<std::vector<uint8_t>>` and if the vector is empty that is the same as the optional without a value.
(https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919)
`std::vector<uint8_t>` can be used instead of `std::optional<std::vector<uint8_t>>` and if the vector is empty that is the same as the optional without a value.