💬 laanwj commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2014230188)
"need to be" instead of "should be" (it's not only because of the size, AFAIK bitcoin core just can't parse text files)
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2014230188)
"need to be" instead of "should be" (it's not only because of the size, AFAIK bitcoin core just can't parse text files)
💬 TheCharlatan commented on pull request "test: Add functional test for bitcoin-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32145#issuecomment-2754565797)
Thank you for the review @laanwj,
Updated 76449b9cdb367eed05cfdf5fe550fc41e477a7fe -> ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 ([BitcoinChainstateFunctionalTest_0](https://github.com/TheCharlatan/bitcoin/tree/BitcoinChainstateFunctionalTest_0) -> [BitcoinChainstateFunctionalTest_1](https://github.com/TheCharlatan/bitcoin/tree/BitcoinChainstateFunctionalTest_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/BitcoinChainstateFunctionalTest_0..BitcoinChainstateFunctionalTest_1))
*
...
(https://github.com/bitcoin/bitcoin/pull/32145#issuecomment-2754565797)
Thank you for the review @laanwj,
Updated 76449b9cdb367eed05cfdf5fe550fc41e477a7fe -> ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 ([BitcoinChainstateFunctionalTest_0](https://github.com/TheCharlatan/bitcoin/tree/BitcoinChainstateFunctionalTest_0) -> [BitcoinChainstateFunctionalTest_1](https://github.com/TheCharlatan/bitcoin/tree/BitcoinChainstateFunctionalTest_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/BitcoinChainstateFunctionalTest_0..BitcoinChainstateFunctionalTest_1))
*
...
💬 ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2754567252)
> But I only have a vague idea of how this should get used in practice. It would be helpful to add some documentation to https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md to say how/when you could run the fuzz binary in debug mode, and when you would might want to set the FUZZ_NONDETERMINISM environment variable.
I think there's two reasons to do this:
* It's an easier way to reproduce most fuzzer crashes (ie, ones that don't need a sanitizer to trigger); just grab your existi
...
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2754567252)
> But I only have a vague idea of how this should get used in practice. It would be helpful to add some documentation to https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md to say how/when you could run the fuzz binary in debug mode, and when you would might want to set the FUZZ_NONDETERMINISM environment variable.
I think there's two reasons to do this:
* It's an easier way to reproduce most fuzzer crashes (ie, ones that don't need a sanitizer to trigger); just grab your existi
...
💬 willcl-ark commented on pull request "Accept unordered tracepoints in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/32101#discussion_r2014260177)
Good idea. Added a comment in 248fdd88dcf651a0560a3eedfb8af61f38c61400
`git range-diff 693d1e2f54baa0d5e407153f79b2f98385e6b8d9...248fdd88dcf651a0560a3eedfb8af61f38c61400`
(https://github.com/bitcoin/bitcoin/pull/32101#discussion_r2014260177)
Good idea. Added a comment in 248fdd88dcf651a0560a3eedfb8af61f38c61400
`git range-diff 693d1e2f54baa0d5e407153f79b2f98385e6b8d9...248fdd88dcf651a0560a3eedfb8af61f38c61400`
💬 rkrux commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2754586784)
One reason I find deduplicating here appealing is that these (de)compress/deserialize utility functions have quite a custom implementation with lack of comments/explanation. My preference would be to have them present once only instead of seeing the implementation in multiple places in the repo.
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2754586784)
One reason I find deduplicating here appealing is that these (de)compress/deserialize utility functions have quite a custom implementation with lack of comments/explanation. My preference would be to have them present once only instead of seeing the implementation in multiple places in the repo.
💬 glozow commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2754605468)
Thanks for the reminder @maflcko. I posted to [delving](https://delvingbitcoin.org/t/bitcoin-core-29-0-release-candidate-is-available/1536) but not sure if the mailing list(s) are still active. Just posted to the google group. Will remember to comment here in the future.
Fwiw rc2 bins are here: https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc2/
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2754605468)
Thanks for the reminder @maflcko. I posted to [delving](https://delvingbitcoin.org/t/bitcoin-core-29-0-release-candidate-is-available/1536) but not sure if the mailing list(s) are still active. Just posted to the google group. Will remember to comment here in the future.
Fwiw rc2 bins are here: https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc2/
💬 laanwj commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2014295306)
i don't think this command is correct if you want to pipe in via stdin, as `$()` puts the data on the command line? do you mean
```
bitcoin-cli getnodeaddresses 0 | python3 asmap-tool.py diff_addrs path/to/first.file path/to/second.file
```
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2014295306)
i don't think this command is correct if you want to pipe in via stdin, as `$()` puts the data on the command line? do you mean
```
bitcoin-cli getnodeaddresses 0 | python3 asmap-tool.py diff_addrs path/to/first.file path/to/second.file
```
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2014298202)
Thanks for re-reviewing this again!
Yes, in that way we would make `error_num == None` be one of the cases for which we `raise`. It's obviously better to `raise` for something unexpected than to suppress, which should lead to it being tracked down and intentionally categorized for suppression or not.
Updated in latest push.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2014298202)
Thanks for re-reviewing this again!
Yes, in that way we would make `error_num == None` be one of the cases for which we `raise`. It's obviously better to `raise` for something unexpected than to suppress, which should lead to it being tracked down and intentionally categorized for suppression or not.
Updated in latest push.
💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2754631442)
> * It lets you include the fuzz test corpora (assuming they all pass) in a regular code coverage build, something like:
Maybe I am missing something, but I guess you meant "debug mode coverage build" instead of "regular code coverage build", because the regular coverage build has neither fuzz, nor debug enabled. For me it looks something like:
```
C++ compiler .......................... GNU 14.2.0, /usr/bin/g++
CMAKE_BUILD_TYPE ...................... Coverage
Preprocessor defined macro
...
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2754631442)
> * It lets you include the fuzz test corpora (assuming they all pass) in a regular code coverage build, something like:
Maybe I am missing something, but I guess you meant "debug mode coverage build" instead of "regular code coverage build", because the regular coverage build has neither fuzz, nor debug enabled. For me it looks something like:
```
C++ compiler .......................... GNU 14.2.0, /usr/bin/g++
CMAKE_BUILD_TYPE ...................... Coverage
Preprocessor defined macro
...
💬 fjahr commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2754631581)
I am currently having similar problems (also on macos). I am not sure if I want to change the docs though, obviously it would be great if we could figure out the actual issue instead.
Maybe a bit more context that I can add, I am seeing the same error as above when running without providing a corpus but with a corpus I see this:
```
$ FUZZ=base32_encode_decode build_fuzz/bin/fuzz ../qa-assets/fuzz_corpora/base32_encode_decode/
fuzz(54938,0x1fedb8840) malloc: nano zone abandoned due to in
...
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2754631581)
I am currently having similar problems (also on macos). I am not sure if I want to change the docs though, obviously it would be great if we could figure out the actual issue instead.
Maybe a bit more context that I can add, I am seeing the same error as above when running without providing a corpus but with a corpus I see this:
```
$ FUZZ=base32_encode_decode build_fuzz/bin/fuzz ../qa-assets/fuzz_corpora/base32_encode_decode/
fuzz(54938,0x1fedb8840) malloc: nano zone abandoned due to in
...
💬 laanwj commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2754653487)
> This script is a part of the CI tests via the functional tests here:
OK, good!
> My preference would be to have them present once only instead of seeing the implementation in multiple places in the repo.
Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools, this makes it clear that there's an interface at least.
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2754653487)
> This script is a part of the CI tests via the functional tests here:
OK, good!
> My preference would be to have them present once only instead of seeing the implementation in multiple places in the repo.
Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools, this makes it clear that there's an interface at least.
💬 laanwj commented on pull request "Accept unordered tracepoints in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/32101#discussion_r2014328129)
Thanks!
(https://github.com/bitcoin/bitcoin/pull/32101#discussion_r2014328129)
Thanks!
💬 maflcko commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2754665676)
A post to one of either delving or the google group should be sufficient. Thanks!
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2754665676)
A post to one of either delving or the google group should be sufficient. Thanks!
👍 laanwj approved a pull request: "Accept unordered tracepoints in interface_usdt_utxocache.py"
(https://github.com/bitcoin/bitcoin/pull/32101#pullrequestreview-2717503724)
Code review ACK 248fdd88dcf651a0560a3eedfb8af61f38c61400
(https://github.com/bitcoin/bitcoin/pull/32101#pullrequestreview-2717503724)
Code review ACK 248fdd88dcf651a0560a3eedfb8af61f38c61400
🤔 rkrux reviewed a pull request: "test: remove strict restrictions on rpc_deprecated test"
(https://github.com/bitcoin/bitcoin/pull/32139#pullrequestreview-2717521581)
I would suggest updating the commit message according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
(https://github.com/bitcoin/bitcoin/pull/32139#pullrequestreview-2717521581)
I would suggest updating the commit message according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
💬 rkrux commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014339327)
Is creating wallet necessary? It didn't seem to be earlier.
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014339327)
Is creating wallet necessary? It didn't seem to be earlier.
💬 polespinasa commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014349640)
Seems to, if the wallet is not created the following error is thrown on the assert: `No wallet is loaded. Load a wallet using loadwallet or create a new one with createwallet. (Note: A default wallet is no longer automatically created) (-18)
`
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014349640)
Seems to, if the wallet is not created the following error is thrown on the assert: `No wallet is loaded. Load a wallet using loadwallet or create a new one with createwallet. (Note: A default wallet is no longer automatically created) (-18)
`
👍 laanwj approved a pull request: "test: Add functional test for bitcoin-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717540435)
Code review ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717540435)
Code review ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2754721275)
@ryanofsky, I've experimented with your version, turns out I'm mostly fine with both solutions. https://github.com/bitcoin/bitcoin/compare/2c073b90ae977af1ac01164a894edd978529fdc6..06f37689a3b79f2f65ac95d8ae54a407ed2d4502
I have based this version on the patch you've posted - with minor differences.
* Removed all introduced `undo_size` seek backs - except in `ReadRawBlock`, which already steps back to get access to the size. I have cleaned it up in a separate commit - we don't need bufferi
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2754721275)
@ryanofsky, I've experimented with your version, turns out I'm mostly fine with both solutions. https://github.com/bitcoin/bitcoin/compare/2c073b90ae977af1ac01164a894edd978529fdc6..06f37689a3b79f2f65ac95d8ae54a407ed2d4502
I have based this version on the patch you've posted - with minor differences.
* Removed all introduced `undo_size` seek backs - except in `ReadRawBlock`, which already steps back to get access to the size. I have cleaned it up in a separate commit - we don't need bufferi
...
💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2754743062)
added tests & rebased on master
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2754743062)
added tests & rebased on master