💬 kevkevinpal commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1861146424)
reACK [1757452](https://github.com/bitcoin/bitcoin/pull/29037/commits/1757452cc55a6dacc62e4258043ee4d711fd281a)
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1861146424)
reACK [1757452](https://github.com/bitcoin/bitcoin/pull/29037/commits/1757452cc55a6dacc62e4258043ee4d711fd281a)
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861203074)
> This is factually incorrect.
Node operators constantly filter out transactions even if they follow the consensus rules as per Bitcoin Core defaults, for example, if OP_RETURN > 83 byte, if mempoolminfee and minrelaytxfee are < 0.00001000 BTC, if mempoolsize > 300 MB (and the minimum fee rises consequently).
- OP_RETURN limits do not make sense. They might get removed in core or users can run binaries provided by @petertodd soon
- `mempoolminfee` and `minrelaytxfee` exist to avoid DoS
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861203074)
> This is factually incorrect.
Node operators constantly filter out transactions even if they follow the consensus rules as per Bitcoin Core defaults, for example, if OP_RETURN > 83 byte, if mempoolminfee and minrelaytxfee are < 0.00001000 BTC, if mempoolsize > 300 MB (and the minimum fee rises consequently).
- OP_RETURN limits do not make sense. They might get removed in core or users can run binaries provided by @petertodd soon
- `mempoolminfee` and `minrelaytxfee` exist to avoid DoS
...
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861269916)
> * OP_RETURN limits do not make sense. They might get removed in core
However, it is very effective. Why will it make no sense?
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861269916)
> * OP_RETURN limits do not make sense. They might get removed in core
However, it is very effective. Why will it make no sense?
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1861447165)
[06cdad4 ](https://github.com/bitcoin/bitcoin/commit/06cdad46fb93f9cbe5badd6d3f53ebbd14299bae)to [00e5986](https://github.com/bitcoin/bitcoin/commit/00e5986d20b7a3497682e69605f2bc3471549c84):
- added missing `return` to `ProcessFixedSeeds()` in case where we already have addrs for all networks (thanks @stratospher for pointing me towards it)
- added a commit to remove `add_fixed_seeds` from `ProcessFixedSeeds()`, since the entire function is gated on this anyway.
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1861447165)
[06cdad4 ](https://github.com/bitcoin/bitcoin/commit/06cdad46fb93f9cbe5badd6d3f53ebbd14299bae)to [00e5986](https://github.com/bitcoin/bitcoin/commit/00e5986d20b7a3497682e69605f2bc3471549c84):
- added missing `return` to `ProcessFixedSeeds()` in case where we already have addrs for all networks (thanks @stratospher for pointing me towards it)
- added a commit to remove `add_fixed_seeds` from `ProcessFixedSeeds()`, since the entire function is gated on this anyway.
💬 ThongchaiDonWanon commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1861500365)
[](url)5M: 6.71% $2.59K 20/8
1H: -4.43% $77.7K 354/227
1D: 144% $545K 2.7K/1.7K
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1861500365)
[](url)5M: 6.71% $2.59K 20/8
1H: -4.43% $77.7K 354/227
1D: 144% $545K 2.7K/1.7K
💬 achow101 commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861506107)
It looks like the `SQLiteBatch` being used to write data to disk is being destroyed unexpectedly, which causes subsequent writes using the same object(??) to have the wrong db transaction state that triggers this error.
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861506107)
It looks like the `SQLiteBatch` being used to write data to disk is being destroyed unexpectedly, which causes subsequent writes using the same object(??) to have the wrong db transaction state that triggers this error.
💬 furszy commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861732127)
Yeah. The description is accurate achow. I had something for this in the past for #25297. We currently support a single db connection at time; when multiple `WalletBatch` are created on different threads, the first one destructed has priority over the others.
IIRC, I had a database manager class providing the db references, mapping the thread id to the db ref. So different threads can wait until other processes finish dumping data, acting accordantly, before making use of the db.
I think I n
...
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861732127)
Yeah. The description is accurate achow. I had something for this in the past for #25297. We currently support a single db connection at time; when multiple `WalletBatch` are created on different threads, the first one destructed has priority over the others.
IIRC, I had a database manager class providing the db references, mapping the thread id to the db ref. So different threads can wait until other processes finish dumping data, acting accordantly, before making use of the db.
I think I n
...
💬 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.