💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430700959)
In 5536ace894e67322161a649f998265f461fbc8ad: I think we can improve the "No external signer ScriptPubKeyManager" message, it doesn't sound intuitive. First time I tested it prior reading the code, I got this error and I thought I didn't have any external signer, so I checked it with `enumerate`.
Perhaps: "There is no ScriptPubKeyManager for this address".
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430700959)
In 5536ace894e67322161a649f998265f461fbc8ad: I think we can improve the "No external signer ScriptPubKeyManager" message, it doesn't sound intuitive. First time I tested it prior reading the code, I got this error and I thought I didn't have any external signer, so I checked it with `enumerate`.
Perhaps: "There is no ScriptPubKeyManager for this address".
💬 MarnixCroes commented on pull request "Fix: Ensure 'Transaction View' remains disabled if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861782254)
> > I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
>
> I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:
> check display here
>
> First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the
...
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861782254)
> > I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
>
> I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:
> check display here
>
> First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the
...
📝 achow101 opened a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112)
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transa
...
(https://github.com/bitcoin/bitcoin/pull/29112)
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transa
...
💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1430727818)
Yes, you are right, and I could confirm this with two of my nodes: If we connect via v2 peer and are disconnected because they have banned us, we would try once again with v1 (and then get disconnected again). Both for automatic connections, and (with this PR) manual ones. Although for manual ones it arguably probably matters less because we'll continuously retry with `-connect` and `-addnode` cli args anyway, even when everything is v1.
I don't really see a good way to avoid this, it's not lik
...
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1430727818)
Yes, you are right, and I could confirm this with two of my nodes: If we connect via v2 peer and are disconnected because they have banned us, we would try once again with v1 (and then get disconnected again). Both for automatic connections, and (with this PR) manual ones. Although for manual ones it arguably probably matters less because we'll continuously retry with `-connect` and `-addnode` cli args anyway, even when everything is v1.
I don't really see a good way to avoid this, it's not lik
...
⚠️ ajtowns opened an issue: "Test case for spending bare multisig?"
(https://github.com/bitcoin/bitcoin/issues/29113)
### Please describe the feature you'd like to see added.
I think the only place we currently test that spending a bare multisig utxo is okay is in `AreInputsStandard` in `script_p2sh_tests.cpp`, but this only checks that `AreInputsStandard()` passes, it doesn't check that there isn't some other rule preventing the spend from entering the mempool and eventually being mined.
Adding
```python
self.log.info('Spending a confirmed bare multisig is okay')
node = self.nodes[0]
...
(https://github.com/bitcoin/bitcoin/issues/29113)
### Please describe the feature you'd like to see added.
I think the only place we currently test that spending a bare multisig utxo is okay is in `AreInputsStandard` in `script_p2sh_tests.cpp`, but this only checks that `AreInputsStandard()` passes, it doesn't check that there isn't some other rule preventing the spend from entering the mempool and eventually being mined.
Adding
```python
self.log.info('Spending a confirmed bare multisig is okay')
node = self.nodes[0]
...
💬 pablomartin4btc commented on pull request "Fix: Ensure 'Transaction View' remains disabled if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861954800)
> Being only able to change it requires a wallet to be open, to me would feel like a bug, no? that's more my "concern"
Fair enough, thanks for sharing your insights.
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861954800)
> Being only able to change it requires a wallet to be open, to me would feel like a bug, no? that's more my "concern"
Fair enough, thanks for sharing your insights.
💬 Fiach-Dubh commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861960853)
ACK
There is evidence that a majority of people do not like or want this behavior. Having it on by default for all node runners, seems contrary to what most node runners would choose as policy if they were consciously given a choice.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861960853)
ACK
There is evidence that a majority of people do not like or want this behavior. Having it on by default for all node runners, seems contrary to what most node runners would choose as policy if they were consciously given a choice.
🤔 amitiuttarwar reviewed a pull request: "test: create deterministic addrman in the functional tests"
(https://github.com/bitcoin/bitcoin/pull/29007#pullrequestreview-1787928129)
approach ACK. yay for finally having addrman determinism in the functional tests 🙌🏽
couple thoughts to consider-
#### 1. clearer subtest addrman setup in `rpc_net.py`
@0xB10C
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
>
agreed. right now, the 3 subtests (`test_addpeeraddress`, `test_getaddrmaninfo` & `test_getrawaddrman`) have a lot of implicit dependencies, making the overall test harder to understa
...
(https://github.com/bitcoin/bitcoin/pull/29007#pullrequestreview-1787928129)
approach ACK. yay for finally having addrman determinism in the functional tests 🙌🏽
couple thoughts to consider-
#### 1. clearer subtest addrman setup in `rpc_net.py`
@0xB10C
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
>
agreed. right now, the 3 subtests (`test_addpeeraddress`, `test_getaddrmaninfo` & `test_getrawaddrman`) have a lot of implicit dependencies, making the overall test harder to understa
...
💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430791494)
these init params are added here, but are only relevant until a couple subtests later in `test_getaddrmaninfo`. having unrelated init params can lead to unexpected behaviors, like in this recent [example](https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016). implicit dependencies between subtests can also make future development confusing, such as updating setup in an earlier test unexpectedly causing failures in seemingly unrelated tests.
leaving suggestions for improving t
...
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430791494)
these init params are added here, but are only relevant until a couple subtests later in `test_getaddrmaninfo`. having unrelated init params can lead to unexpected behaviors, like in this recent [example](https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016). implicit dependencies between subtests can also make future development confusing, such as updating setup in an earlier test unexpectedly causing failures in seemingly unrelated tests.
leaving suggestions for improving t
...
💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430801807)
I think it makes sense to update the comment further. here's why-
on current master, this test sets up addrman by putting 1 addr into tried, followed by 1 addr into new. the comment explains to future devs why this setup must be maintained.
this patch doesn't update the setup in the `addpeeraddress` test. the comment on master is outdated because we no longer need to preserve the careful ordering, but I find the currently proposed comment misleading since the deterministic addrman isn't y
...
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430801807)
I think it makes sense to update the comment further. here's why-
on current master, this test sets up addrman by putting 1 addr into tried, followed by 1 addr into new. the comment explains to future devs why this setup must be maintained.
this patch doesn't update the setup in the `addpeeraddress` test. the comment on master is outdated because we no longer need to preserve the careful ordering, but I find the currently proposed comment misleading since the deterministic addrman isn't y
...
💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430822958)
I like the approach of using `-test` to group test-only options
two suggestions for improvement:
* include the options in the help text (eg. how `-onlynet` or `-debug` handles)
* throw some sort of error (or at least warning?) if an invalid option is passed through
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430822958)
I like the approach of using `-test` to group test-only options
two suggestions for improvement:
* include the options in the help text (eg. how `-onlynet` or `-debug` handles)
* throw some sort of error (or at least warning?) if an invalid option is passed through
💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1430841261)
@jonatack said that I should run the script in this [comment](https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1406322235) and it lead to there being a indentation, I can remove if needed
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1430841261)
@jonatack said that I should run the script in this [comment](https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1406322235) and it lead to there being a indentation, I can remove if needed
🤔 stratospher reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-1788195228)
ACK dfd635b.
i think this is a good improvement to have! - better peer diversity compared to what's happening on master where we directly open connections to hardcoded seeds.
tested it using `getaddrmaninfo` RPC
- before fixed seeds were added, new table already had around 1679 addresses from the addrfetch connections
- after fixed seeds were added, new table had 2399 addresses
observed 2 addrfetch connections being successfully opened during the 2 minute delay though.
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-1788195228)
ACK dfd635b.
i think this is a good improvement to have! - better peer diversity compared to what's happening on master where we directly open connections to hardcoded seeds.
tested it using `getaddrmaninfo` RPC
- before fixed seeds were added, new table already had around 1679 addresses from the addrfetch connections
- after fixed seeds were added, new table had 2399 addresses
observed 2 addrfetch connections being successfully opened during the 2 minute delay though.
💬 stratospher commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430982146)
7cc4633: could also update the doc comments to `Load addresses of peers from the DNS and from fixed seeds.`
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430982146)
7cc4633: could also update the doc comments to `Load addresses of peers from the DNS and from fixed seeds.`
💬 stratospher commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430984088)
611923e: we could remove this fixed seed logic handled in `ThreadOpenConnections` comment.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1430984088)
611923e: we could remove this fixed seed logic handled in `ThreadOpenConnections` comment.
💬 maflcko commented on issue "Test case for spending bare multisig?":
(https://github.com/bitcoin/bitcoin/issues/29113#issuecomment-1862310039)
pull request welcome :)
(https://github.com/bitcoin/bitcoin/issues/29113#issuecomment-1862310039)
pull request welcome :)
💬 maflcko commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1862494786)
```
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_descriptor.py", line 68, in test_concurrent_writes
assert_equal(cache_records, 10000)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not
...
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1862494786)
```
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_descriptor.py", line 68, in test_concurrent_writes
assert_equal(cache_records, 10000)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not
...
💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862522407)
> `ArrayLike` sounds like `ranges::contiguous_range` ? https://en.cppreference.com/w/cpp/ranges/contiguous_range
Not exactly. `contiguous_range` is satisfied for `std::vector<int>`, whose size is runtime variable. My `concept ArrayLike` should be `contiguous_range` *and* checking that the size is known at compile time.
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862522407)
> `ArrayLike` sounds like `ranges::contiguous_range` ? https://en.cppreference.com/w/cpp/ranges/contiguous_range
Not exactly. `contiguous_range` is satisfied for `std::vector<int>`, whose size is runtime variable. My `concept ArrayLike` should be `contiguous_range` *and* checking that the size is known at compile time.
💬 maflcko commented on pull request "Update doc/policy/README.md":
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1862556757)
Are you still working on this, or can this be closed up for grabs?
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1862556757)
Are you still working on this, or can this be closed up for grabs?
💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862581792)
https://stackoverflow.com/questions/70482497/detecting-compile-time-constantness-of-range-size ? `std::span<X, N>` seems like it's already pretty much the thing that tells you you've got a contiguous range of exactly N X's to me though...
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1862581792)
https://stackoverflow.com/questions/70482497/detecting-compile-time-constantness-of-range-size ? `std::span<X, N>` seems like it's already pretty much the thing that tells you you've got a contiguous range of exactly N X's to me though...