š¬ hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012886462)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011736752:
Going with this for now:
```suggestion
for (uint32_t value : multipath->values) {
```
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012886462)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011736752:
Going with this for now:
```suggestion
for (uint32_t value : multipath->values) {
```
š¬ hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012885306)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011759617:
Splitting validation from usage sounds good, but l0rincs version introduces more state and as rkrux mentioned still validates the parsing of the numbers in the "non-validating" loop. It's a bit more drastic than I was aiming for and still feels incomplete.
That version also undoes the move to keep the multipath data together towards the end, see list item 3 in the PR description. In the beginning it also uses `std::pai
...
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012885306)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011759617:
Splitting validation from usage sounds good, but l0rincs version introduces more state and as rkrux mentioned still validates the parsing of the numbers in the "non-validating" loop. It's a bit more drastic than I was aiming for and still feels incomplete.
That version also undoes the move to keep the multipath data together towards the end, see list item 3 in the PR description. In the beginning it also uses `std::pai
...
š¬ l0rinc commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752475724)
I have opened https://github.com/bitcoin/bitcoin/pull/32141 - but still [can't fuzz properly on mac](https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752412513), so would appreciate if someone could run it for a few minutes (especially on Windows).
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752475724)
I have opened https://github.com/bitcoin/bitcoin/pull/32141 - but still [can't fuzz properly on mac](https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752412513), so would appreciate if someone could run it for a few minutes (especially on Windows).
š¬ l0rinc commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2752494837)
ACK bd34c6008aa455d5b294db8977be471d1bc837df - not yet sure about the last commit, let's see what others think
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2752494837)
ACK bd34c6008aa455d5b294db8977be471d1bc837df - not yet sure about the last commit, let's see what others think
š¬ maflcko commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752503087)
> I've also added an optional `PSBT_MAGIC_BYTES` prefix, otherwise the the fuzzer couldn't even crack this condition https://github.com/bitcoin/bitcoin/blob/master/src/psbt.h#L1043 in time.
> After this change the fuzzer is finally stressing the `PartiallySignedTransaction` deserialization.
If a fuzz engine can't crack a 5-byte constant byte literal, then something must be wrong.
According to https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/dfb7d58108daf372/c62d
...
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752503087)
> I've also added an optional `PSBT_MAGIC_BYTES` prefix, otherwise the the fuzzer couldn't even crack this condition https://github.com/bitcoin/bitcoin/blob/master/src/psbt.h#L1043 in time.
> After this change the fuzzer is finally stressing the `PartiallySignedTransaction` deserialization.
If a fuzz engine can't crack a 5-byte constant byte literal, then something must be wrong.
According to https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/dfb7d58108daf372/c62d
...
š¬ murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2012925701)
`0xe280af` is the UTF-8 encoding for a narrow no-break space: https://www.fileformat.info/info/unicode/char/202f/index.htm
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2012925701)
`0xe280af` is the UTF-8 encoding for a narrow no-break space: https://www.fileformat.info/info/unicode/char/202f/index.htm
š¬ naiyoma commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#issuecomment-2752525986)
@musaHaruna, thanks for the review,
@ismaelsadeeq I've broken down commits to match description
(https://github.com/bitcoin/bitcoin/pull/32079#issuecomment-2752525986)
@musaHaruna, thanks for the review,
@ismaelsadeeq I've broken down commits to match description
ā
glozow closed an issue: "fuzz: package_rbf crashes after out-of-range memory read"
(https://github.com/bitcoin/bitcoin/issues/32121)
(https://github.com/bitcoin/bitcoin/issues/32121)
š glozow merged a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122)
(https://github.com/bitcoin/bitcoin/pull/32122)
š¬ glozow commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2752535143)
backported in #32136
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2752535143)
backported in #32136
š¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2752590597)
- Following the discussion , I have removed the `-DEBUG` flag .
- Added `-Xdemangler=llvm-cxxfilt` option for more readable outputs.
- Added the note to consider with a clean state to avoid these issues https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1966580329
- Added some suggestions by @jonatack
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2752590597)
- Following the discussion , I have removed the `-DEBUG` flag .
- Added `-Xdemangler=llvm-cxxfilt` option for more readable outputs.
- Added the note to consider with a clean state to avoid these issues https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1966580329
- Added some suggestions by @jonatack
š¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2012968058)
Decided against it since the previous docs built the coverage in `build` directory .
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2012968058)
Decided against it since the previous docs built the coverage in `build` directory .
š¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2012968720)
Added , although I prefer it being before the commands then after it.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2012968720)
Added , although I prefer it being before the commands then after it.
š¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2752595502)
@ryanofsky I have addressed the remaining comments and these should be my last changes (if everyone agrees)
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2752595502)
@ryanofsky I have addressed the remaining comments and these should be my last changes (if everyone agrees)
š¬ l0rinc commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752598212)
> What are the exact steps to reproduce that you took to confirm it isn't covered and unreachable?
Not unreachable, just "couldn't even crack this condition [...] in time."
```C++
// PartiallySignedTransaction
inline void Unserialize(Stream& s) {
// Read the magic bytes
uint8_t magic[5];
s >> magic;
if (!std::equal(magic, magic + 5, PSBT_MAGIC_BYTES)) {
throw std::ios_base::failure("Invalid PSBT magic bytes");
}
th
...
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752598212)
> What are the exact steps to reproduce that you took to confirm it isn't covered and unreachable?
Not unreachable, just "couldn't even crack this condition [...] in time."
```C++
// PartiallySignedTransaction
inline void Unserialize(Stream& s) {
// Read the magic bytes
uint8_t magic[5];
s >> magic;
if (!std::equal(magic, magic + 5, PSBT_MAGIC_BYTES)) {
throw std::ios_base::failure("Invalid PSBT magic bytes");
}
th
...
š¬ Jojad9 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/727c54276967e001f43831db83c790a1dec7a598#commitcomment-154317895)
build_freebsd_SHA256SUM
(https://github.com/bitcoin/bitcoin/commit/727c54276967e001f43831db83c790a1dec7a598#commitcomment-154317895)
build_freebsd_SHA256SUM
š¬ ariard commented on issue "Consideration of a Code of Conduct and/or written rules for moderation":
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2752618771)
> We should be willing to discuss concrete, real-world, examples where a proposed Code of Conduct would be relevant. While this is a particularly extreme example, it certainly is relevant given that it is alleged to have happened, with people closely involved in the repo. It raises an important point: is conduct outside of the direct communication of the project relevant?
Re-opened a pull request to remove the current moderation rules on `bitcoin-core-meta`.
Iām not going to comment on what di
...
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2752618771)
> We should be willing to discuss concrete, real-world, examples where a proposed Code of Conduct would be relevant. While this is a particularly extreme example, it certainly is relevant given that it is alleged to have happened, with people closely involved in the repo. It raises an important point: is conduct outside of the direct communication of the project relevant?
Re-opened a pull request to remove the current moderation rules on `bitcoin-core-meta`.
Iām not going to comment on what di
...
š¬ maflcko commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752621238)
Running fuzzing in a single thread, for less than 4 minutes, with an empty fuzz input folder isn't representative.
My recommendation would be to run in in more threads, for longer time, and with the existing fuzz inputs.
If you really want to start with an empty folder, you can try `-use_value_profile=1`.
If you want to help the fuzz engine with the encoding, it seems best to do over the whole input and not only the prefix.
In any case, I don't see why this should be bundled in this
...
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752621238)
Running fuzzing in a single thread, for less than 4 minutes, with an empty fuzz input folder isn't representative.
My recommendation would be to run in in more threads, for longer time, and with the existing fuzz inputs.
If you really want to start with an empty folder, you can try `-use_value_profile=1`.
If you want to help the fuzz engine with the encoding, it seems best to do over the whole input and not only the prefix.
In any case, I don't see why this should be bundled in this
...
š¤ mzumsande reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2714838048)
I don't completely understand what the second part of the PR (`BESTBLOCK_NOMERKLE` serialization) achieves in the current approach.
With 748063a9820cb740c9b68f80b9e8c97d2a21e83d, `chainStateFlushed` has been made a no-op and could be removed.
On the other hand, the expressed goal of having `m_last_block_processed` and `m_last_block_processed_height` always in sync with the block locator is no longer current, given that we only update every 144 blocks if there is nothing relevant for the walle
...
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2714838048)
I don't completely understand what the second part of the PR (`BESTBLOCK_NOMERKLE` serialization) achieves in the current approach.
With 748063a9820cb740c9b68f80b9e8c97d2a21e83d, `chainStateFlushed` has been made a no-op and could be removed.
On the other hand, the expressed goal of having `m_last_block_processed` and `m_last_block_processed_height` always in sync with the block locator is no longer current, given that we only update every 144 blocks if there is nothing relevant for the walle
...
š¬ mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2012735002)
8b8cb9f4e9055a3283805ed6ff9397236842bb34 :
need to update `wallet_reorgsrestore.py `after #31757 introduced a test for the bug this PR fixes.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2012735002)
8b8cb9f4e9055a3283805ed6ff9397236842bb34 :
need to update `wallet_reorgsrestore.py `after #31757 introduced a test for the bug this PR fixes.