💬 luke-jr commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1872231344)
>...the results may be randomized but must not single-out any group of hosts to receive different results unless due to an urgent technical necessity and disclosed.
To be clear, this refers to giving different results to different requesters. It does not forbid selection of which peers to return as results to everyone, which is quite normal for DNS seeds.
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1872231344)
>...the results may be randomized but must not single-out any group of hosts to receive different results unless due to an urgent technical necessity and disclosed.
To be clear, this refers to giving different results to different requesters. It does not forbid selection of which peers to return as results to everyone, which is quite normal for DNS seeds.
💬 etfmoon commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1872237278)
> To be clear
This pull request was close without any answers and we know the politics involved in it with reasons.
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1872237278)
> To be clear
This pull request was close without any answers and we know the politics involved in it with reasons.
💬 luke-jr commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-1872243366)
>It would be good to list at least one benefit, otherwise the benefits of this change are unclear.
Having a single file to download for the signatures is simpler for end users.
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-1872243366)
>It would be good to list at least one benefit, otherwise the benefits of this change are unclear.
Having a single file to download for the signatures is simpler for end users.
👋 mzumsande's pull request is ready for review: "p2p: Increase inbound capacity for block-relay only connections"
(https://github.com/bitcoin/bitcoin/pull/28463)
(https://github.com/bitcoin/bitcoin/pull/28463)
⚠️ dooglus opened an issue: "new crash in v26.0"
(https://github.com/bitcoin/bitcoin/issues/29153)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
bitcoin-qt crashed while loading wallets at startup.
I used to see occasional crashes on startup a few years ago, but it hasn't been happening at all in the last couple of major releases.
I've been running v26.0 for a week or two and haven't had any problem with it crashing until today.
Here's a backtrace. I run it in gdb habitually because I used to see a lot of crashes and never
...
(https://github.com/bitcoin/bitcoin/issues/29153)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
bitcoin-qt crashed while loading wallets at startup.
I used to see occasional crashes on startup a few years ago, but it hasn't been happening at all in the last couple of major releases.
I've been running v26.0 for a week or two and haven't had any problem with it crashing until today.
Here's a backtrace. I run it in gdb habitually because I used to see a lot of crashes and never
...
💬 murchandamus commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1438397150)
The code seems correct to me, but the test seems to have failed on line 408:
https://cirrus-ci.com/task/6362440724643840?logs=ci#L2630
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1438397150)
The code seems correct to me, but the test seems to have failed on line 408:
https://cirrus-ci.com/task/6362440724643840?logs=ci#L2630
📝 mjdietzx opened a pull request: "tests: improve wallet multisig descriptor test and docs"
(https://github.com/bitcoin/bitcoin/pull/29154)
It is best to store all key origin information
(master key fingerprint and all derivation steps)
in the multisig descriptor. Being explicit with
this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others.
<!--
*** Please remove the following help text
...
(https://github.com/bitcoin/bitcoin/pull/29154)
It is best to store all key origin information
(master key fingerprint and all derivation steps)
in the multisig descriptor. Being explicit with
this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others.
<!--
*** Please remove the following help text
...
💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1438410355)
I'm not sure what you are referring to. Could you rephrase it please.
You want to update `pindexLastCommonBlock` to `pindex`?
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1438410355)
I'm not sure what you are referring to. Could you rephrase it please.
You want to update `pindexLastCommonBlock` to `pindex`?
✅ dimitaracev closed a pull request: "wallet: move lock to the top of ReleaseWallet"
(https://github.com/bitcoin/bitcoin/pull/29143)
(https://github.com/bitcoin/bitcoin/pull/29143)
📝 dimitaracev opened a pull request: "wallet: move lock at the top of ReleaseWallet"
(https://github.com/bitcoin/bitcoin/pull/29155)
Fixes https://github.com/bitcoin/bitcoin/issues/29073
(https://github.com/bitcoin/bitcoin/pull/29155)
Fixes https://github.com/bitcoin/bitcoin/issues/29073
💬 dimitaracev commented on pull request "wallet: move lock to the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1872344129)
Closed in favor of https://github.com/bitcoin/bitcoin/pull/29155 due to the branch name being misleading.
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1872344129)
Closed in favor of https://github.com/bitcoin/bitcoin/pull/29155 due to the branch name being misleading.
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438415389)
> The `g_printed_dir` guard is only needed because the user may be running more than one test case
This would break the PR goal in the current implementation. Only the last executed test datadir would be kept, the rest will be deleted.
I think we should include the test name in the path and create a datadir directory for each executed tests. Could pull most of what I did before: 53eb4980d561491798d0c8b0ee383c83ba193432.
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438415389)
> The `g_printed_dir` guard is only needed because the user may be running more than one test case
This would break the PR goal in the current implementation. Only the last executed test datadir would be kept, the rest will be deleted.
I think we should include the test name in the path and create a datadir directory for each executed tests. Could pull most of what I did before: 53eb4980d561491798d0c8b0ee383c83ba193432.
📝 mjdietzx opened a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156)
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
<!--
*** Please remove the following help text before submitting:
...
(https://github.com/bitcoin/bitcoin/pull/29156)
This is very closely based on [test/functional/wallet_multisig_descriptor_psbt.py](/test/functional/wallet_multisig_descriptor_psbt.py) both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).
<!--
*** Please remove the following help text before submitting:
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1872346963)
In a [simulation](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) comparing the `master` branch with the `CoinGrinder` branch playing through a scenario that performs about 5 000 payments with feerates sampled from 2023, ___CoinGrinder reduces the total transaction fees by 9.5%___.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1872346963)
In a [simulation](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) comparing the `master` branch with the `CoinGrinder` branch playing through a scenario that performs about 5 000 payments with feerates sampled from 2023, ___CoinGrinder reduces the total transaction fees by 9.5%___.
📝 LarryRuane converted_to_draft a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp` as the working directory
- When the test starts, remove `<datadir>/test_temp` if it exists from an earlier run (currently, it's presumed not to
...
(https://github.com/bitcoin/bitcoin/pull/26564)
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp` as the working directory
- When the test starts, remove `<datadir>/test_temp` if it exists from an earlier run (currently, it's presumed not to
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438429769)
I'd avoid the `std::vector::at` everywhere and use `std::vector::operator[]` instead, as `at` involves conditional branches to detect out-of-bounds access (well-predicted ones, but for a tight loop like this I do expect it to still be impactful). It's generally better to crash than to have UB due to out-of-bounds access, but inside performance-critical algorithms I think it's worth the extra review effort to make sure there are no out-of-bounds accesses.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438429769)
I'd avoid the `std::vector::at` everywhere and use `std::vector::operator[]` instead, as `at` involves conditional branches to detect out-of-bounds access (well-predicted ones, but for a tight loop like this I do expect it to still be impactful). It's generally better to crash than to have UB due to out-of-bounds access, but inside performance-critical algorithms I think it's worth the extra review effort to make sure there are no out-of-bounds accesses.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411976)
It took me a while to figure out what the intent of each operation is. It may be worthwhile to document that here too. My belief is:
* ADD: make the next undecided transaction selected (ignoring equal-value transactions if the last one was unselected).
* SHIFT: the last selected transaction is bad, skip any configurations that select it, and ADD something after that.
* CUT: the last selected transaction is bad even when unselected, skip any configurations that select or unselect it, and ADD s
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411976)
It took me a while to figure out what the intent of each operation is. It may be worthwhile to document that here too. My belief is:
* ADD: make the next undecided transaction selected (ignoring equal-value transactions if the last one was unselected).
* SHIFT: the last selected transaction is bad, skip any configurations that select it, and ADD something after that.
* CUT: the last selected transaction is bad even when unselected, skip any configurations that select or unselect it, and ADD s
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411775)
Perhaps this explanation of the algorithm belongs more inside the body of the function than at the top, as it's explaining the implementation and not the interface?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438411775)
Perhaps this explanation of the algorithm belongs more inside the body of the function than at the top, as it's explaining the implementation and not the interface?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438428556)
Given that any of the {add, shift, cut} states ends with executing an add, and the end of the add operation (the block of code on this line and below) determines what the next iteration will be, I think it would be more natural to move this block to the beginning of the while loop, before the switch case. So then it becomes a processing loop that in every iteration decides what it will be doing (cut, shift, or add), without state carried between iterations? This doesn't work for "done", but that
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438428556)
Given that any of the {add, shift, cut} states ends with executing an add, and the end of the add operation (the block of code on this line and below) determines what the next iteration will be, I think it would be more natural to move this block to the beginning of the while loop, before the switch case. So then it becomes a processing loop that in every iteration decides what it will be doing (cut, shift, or add), without state carried between iterations? This doesn't work for "done", but that
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438435912)
One complication with the "move decision for add/shift/cut to beginning of loop" approach is that this skipping doesn't work anymore as-is. I'd suggest turning it into a while loop that increments `utxo_pool_index` until the skip condition is no longer satisfied.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1438435912)
One complication with the "move decision for add/shift/cut to beginning of loop" approach is that this skipping doesn't work anymore as-is. I'd suggest turning it into a while loop that increments `utxo_pool_index` until the skip condition is no longer satisfied.