💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1449443895)
Updated 88e88a5 -> f87c39399823e22c553b797cc66fa4063462a32b ([removeBlockstorageArgs_5](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_5) -> [removeBlockstorageArgs_6](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_5..removeBlockstorageArgs_6)) with linter fix.
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1449443895)
Updated 88e88a5 -> f87c39399823e22c553b797cc66fa4063462a32b ([removeBlockstorageArgs_5](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_5) -> [removeBlockstorageArgs_6](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_5..removeBlockstorageArgs_6)) with linter fix.
💬 wtogami commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449474465)
Could we please pick a different name for old witnessless syncing than "pruned"? It's different from the old pruned node type that entirely discards old blocks. Old witnessless nodes could reindex and rescan as well as provide old witnessless blocks to other nodes doing IBD of this type. We need a new name for this.
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449474465)
Could we please pick a different name for old witnessless syncing than "pruned"? It's different from the old pruned node type that entirely discards old blocks. Old witnessless nodes could reindex and rescan as well as provide old witnessless blocks to other nodes doing IBD of this type. We need a new name for this.
👍 w0xlt approved a pull request: "doc: Expand scantxoutset help text to cover tr() and miniscript"
(https://github.com/bitcoin/bitcoin/pull/27155)
ACK https://github.com/bitcoin/bitcoin/pull/27155/commits/ca605f015dc8fbabbc6c0640d87429d6bf8f553f
(https://github.com/bitcoin/bitcoin/pull/27155)
ACK https://github.com/bitcoin/bitcoin/pull/27155/commits/ca605f015dc8fbabbc6c0640d87429d6bf8f553f
💬 w0xlt commented on pull request "doc: Expand scantxoutset help text to cover tr() and miniscript":
(https://github.com/bitcoin/bitcoin/pull/27155#discussion_r1121339332)
Could also include `rawtr(<pubkey>) P2TR output with the specified key as output key`.
(https://github.com/bitcoin/bitcoin/pull/27155#discussion_r1121339332)
Could also include `rawtr(<pubkey>) P2TR output with the specified key as output key`.
📝 Sjors opened a pull request: "26032 followups"
(https://github.com/bitcoin/bitcoin/pull/27180)
Followups for #26032. So far nothing major.
(https://github.com/bitcoin/bitcoin/pull/27180)
Followups for #26032. So far nothing major.
💬 Sjors commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1121378441)
Added to a followup PR #27180.
(https://github.com/bitcoin/bitcoin/pull/26032#discussion_r1121378441)
Added to a followup PR #27180.
💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449640124)
I think it's fine to have a temporary hoop here, as long as it's documented.
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449640124)
I think it's fine to have a temporary hoop here, as long as it's documented.
💬 hebasto commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449655488)
Well, speaking of the removed questionable [commit](https://github.com/bitcoin/bitcoin/commit/a25daf76b5c6659e9002d6ae08a7907933fc120b), I've finally managed to provide steps to reproduce unexpected behavior _without_ that commit:
1. Complete all tasks.
2. Re-run, for example, the "32-bit + dash [gui] [CentOS 8]" task. As expected, ccache achieves 100% hit.
3. Add a commit which does _not_ change any code and run CI job. It is natural to expect that ccache will 100% hit this time as well. But
...
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449655488)
Well, speaking of the removed questionable [commit](https://github.com/bitcoin/bitcoin/commit/a25daf76b5c6659e9002d6ae08a7907933fc120b), I've finally managed to provide steps to reproduce unexpected behavior _without_ that commit:
1. Complete all tasks.
2. Re-run, for example, the "32-bit + dash [gui] [CentOS 8]" task. As expected, ccache achieves 100% hit.
3. Add a commit which does _not_ change any code and run CI job. It is natural to expect that ccache will 100% hit this time as well. But
...
💬 Sjors commented on pull request "rpc: make importaddress compatible with descriptors wallet":
(https://github.com/bitcoin/bitcoin/pull/27034#discussion_r1121399359)
> upgraded the legacy wallet into a descriptors wallet
I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.
(https://github.com/bitcoin/bitcoin/pull/27034#discussion_r1121399359)
> upgraded the legacy wallet into a descriptors wallet
I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.
💬 Sjors commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1121419524)
47bc99cddc518dc41741c5dcc17f9e629e162449 This is probably fine, but there's a bunch a places that we have to make sure are never reached. For example in `FindBlockPos(` we do `_blockfile_info[nFile]` where `nFile = ... pos.nFile`. I'll have to review that a bit more.
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1121419524)
47bc99cddc518dc41741c5dcc17f9e629e162449 This is probably fine, but there's a bunch a places that we have to make sure are never reached. For example in `FindBlockPos(` we do `_blockfile_info[nFile]` where `nFile = ... pos.nFile`. I'll have to review that a bit more.
💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449715715)
It would be good to double check if this is recommended "officially" by Cirrus CI. In any case `CIRRUS_WORKING_DIR` isn't under `/tmp/` on macos, so maybe it makes sense for that reason already?
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449715715)
It would be good to double check if this is recommended "officially" by Cirrus CI. In any case `CIRRUS_WORKING_DIR` isn't under `/tmp/` on macos, so maybe it makes sense for that reason already?
💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449717685)
I guess instead of `${CIRRUS_WORKING_DIR}/ccache_dir` you can also just write `ccache_dir`, as `CIRRUS_WORKING_DIR` is implicit?
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1449717685)
I guess instead of `${CIRRUS_WORKING_DIR}/ccache_dir` you can also just write `ccache_dir`, as `CIRRUS_WORKING_DIR` is implicit?
💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449721171)
Ok, fair enough. Also, I guess no one is forced to upgrade their script. Anyone is free to use the previous version of the python script as long as they want.
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1449721171)
Ok, fair enough. Also, I guess no one is forced to upgrade their script. Anyone is free to use the previous version of the python script as long as they want.
💬 glozow commented on pull request "26032 followups":
(https://github.com/bitcoin/bitcoin/pull/27180#discussion_r1121471409)
trailing whitespace here
(https://github.com/bitcoin/bitcoin/pull/27180#discussion_r1121471409)
trailing whitespace here
💬 dergoegge commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449815580)
@wtogami This PR does not introduce a new witnessless archival mode (which is what you are describing iiuc). It only makes pruned nodes skip downloading the witnesses up to the assume-valid point (since they're not validated and deleted shortly after anyway).
I would agree that witnessless archival nodes should not be called "pruned", but again that is not what this PR is introducing.
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1449815580)
@wtogami This PR does not introduce a new witnessless archival mode (which is what you are describing iiuc). It only makes pruned nodes skip downloading the witnesses up to the assume-valid point (since they're not validated and deleted shortly after anyway).
I would agree that witnessless archival nodes should not be called "pruned", but again that is not what this PR is introducing.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121488980)
Marking as resolved as this has been implemented
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121488980)
Marking as resolved as this has been implemented
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121487831)
I agree it might just be better to not reserve anything. We have no idea what the cluster size is going to be, so might as well let the stdlib magic do its work.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121487831)
I agree it might just be better to not reserve anything. We have no idea what the cluster size is going to be, so might as well let the stdlib magic do its work.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121498816)
Disagree with txmempool getting this value from mini_miner, as that would create a circular dependency.
This kind of constant should live in src/kernel/mempool_options.h.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121498816)
Disagree with txmempool getting this value from mini_miner, as that would create a circular dependency.
This kind of constant should live in src/kernel/mempool_options.h.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121483332)
Can you explain why using `count` is better?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1121483332)
Can you explain why using `count` is better?
⚠️ stickies-v opened an issue: "libevent: event_enable_debug_logging() not to be used after creating of event base"
(https://github.com/bitcoin/bitcoin/issues/27182)
Raising this issue on behalf of @hebasto who identified the potential problem as well as dug into the docs.
The `logging` RPC method, through [`UpdateHTTPServerLogging()`](https://github.com/bitcoin/bitcoin/blob/cb40639bdf04bab31bcdceb3bf941e9bade8317a/src/httpserver.cpp#L410-L416) calls the libevent function `event_enable_debug_logging()`. The libevent documentation [states](https://libevent.org/libevent-book/Ref1_libsetup.html) that "You must make any changes to these settings before you ca
...
(https://github.com/bitcoin/bitcoin/issues/27182)
Raising this issue on behalf of @hebasto who identified the potential problem as well as dug into the docs.
The `logging` RPC method, through [`UpdateHTTPServerLogging()`](https://github.com/bitcoin/bitcoin/blob/cb40639bdf04bab31bcdceb3bf941e9bade8317a/src/httpserver.cpp#L410-L416) calls the libevent function `event_enable_debug_logging()`. The libevent documentation [states](https://libevent.org/libevent-book/Ref1_libsetup.html) that "You must make any changes to these settings before you ca
...