β
jonatack closed a pull request: "Severity-based logging -- parent PR"
(https://github.com/bitcoin/bitcoin/pull/25203)
(https://github.com/bitcoin/bitcoin/pull/25203)
π¬ jonatack commented on pull request "Severity-based logging -- parent PR":
(https://github.com/bitcoin/bitcoin/pull/25203#issuecomment-2043169446)
Closing to be able to re-open it.
(https://github.com/bitcoin/bitcoin/pull/25203#issuecomment-2043169446)
Closing to be able to re-open it.
π¬ jonatack commented on pull request "refactor: deduplicate AmountFromValue() functions":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-2043177731)
Will re-open in a new PR.
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-2043177731)
Will re-open in a new PR.
π¬ jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-2043185560)
Will re-open as a new PR, along with the alternative version, as it is a good and mature patch with many updates made for reviews and nits.
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-2043185560)
Will re-open as a new PR, along with the alternative version, as it is a good and mature patch with many updates made for reviews and nits.
π¬ davidgumberg commented on pull request "test: Update --tmpdir doc string to say directory must not exist":
(https://github.com/bitcoin/bitcoin/pull/29498#issuecomment-2043766965)
ACK https://github.com/bitcoin/bitcoin/pull/29498/commits/d4e36ae80d4b3f03647fd9057461edf7ecd794a3
(https://github.com/bitcoin/bitcoin/pull/29498#issuecomment-2043766965)
ACK https://github.com/bitcoin/bitcoin/pull/29498/commits/d4e36ae80d4b3f03647fd9057461edf7ecd794a3
π€ tdb3 reviewed a pull request: "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16"
(https://github.com/bitcoin/bitcoin/pull/28312#pullrequestreview-1987913814)
Great work catching the bug and making testing more robust.
ACK for d113709dc4e68535605d22814ea987b55469bd32.
One inline recommendation to cover an additional related edge case.
(https://github.com/bitcoin/bitcoin/pull/28312#pullrequestreview-1987913814)
Great work catching the bug and making testing more robust.
ACK for d113709dc4e68535605d22814ea987b55469bd32.
One inline recommendation to cover an additional related edge case.
π¬ tdb3 commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1556687636)
Since > 20 keys are not allowed when creating a multisig address, recommend adding a test case to check this.
Something like:
```diff
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 4bbd2a526d..88f4e44a65 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -129,6 +129,9 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
res = self.nodes[0].createmultisig(nrequired=nkeys, keys=
...
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1556687636)
Since > 20 keys are not allowed when creating a multisig address, recommend adding a test case to check this.
Something like:
```diff
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 4bbd2a526d..88f4e44a65 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -129,6 +129,9 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
res = self.nodes[0].createmultisig(nrequired=nkeys, keys=
...
π¬ andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2044108942)
Ran benchmarks to 800k blocks, 2 times each. As expected, results are very good when running pruned. When running non-pruned with default `dbcache`, however, `master` was 1% faster. I think this slowdown is negligible though and the improvements to syncing with pruning enabled make this worth the changes here. Also, potential improvements like https://github.com/bitcoin/bitcoin/pull/28945 will make up for it.
| | prune | dbcache | mean time | speedup |
|-----------:|----------:|------------
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2044108942)
Ran benchmarks to 800k blocks, 2 times each. As expected, results are very good when running pruned. When running non-pruned with default `dbcache`, however, `master` was 1% faster. I think this slowdown is negligible though and the improvements to syncing with pruning enabled make this worth the changes here. Also, potential improvements like https://github.com/bitcoin/bitcoin/pull/28945 will make up for it.
| | prune | dbcache | mean time | speedup |
|-----------:|----------:|------------
...
β
maflcko closed a pull request: "test: randomize perturbed files excluding ldb"
(https://github.com/bitcoin/bitcoin/pull/29182)
(https://github.com/bitcoin/bitcoin/pull/29182)
π¬ maflcko commented on pull request "test: randomize perturbed files excluding ldb":
(https://github.com/bitcoin/bitcoin/pull/29182#issuecomment-2044179883)
Closing for now, but feel free to open a new pull request, with the changes rebased.
(https://github.com/bitcoin/bitcoin/pull/29182#issuecomment-2044179883)
Closing for now, but feel free to open a new pull request, with the changes rebased.
π¬ Ashflower13 commented on pull request "test: Update --tmpdir doc string to say directory must not exist":
(https://github.com/bitcoin/bitcoin/pull/29498#issuecomment-2044195523)
Going to get some help soon I need teams to help Sent from my iPhoneOn Apr 8, 2024, at 15:56, David Gumberg ***@***.***> wrote:ο»Ώ
ACK d4e36ae
βReply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
(https://github.com/bitcoin/bitcoin/pull/29498#issuecomment-2044195523)
Going to get some help soon I need teams to help Sent from my iPhoneOn Apr 8, 2024, at 15:56, David Gumberg ***@***.***> wrote:ο»Ώ
ACK d4e36ae
βReply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
π¬ instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1556951038)
We can't restart the node after filling the memopool without resetting the floating minfee.
I could pull in the initial restart if we knew people didn't want any other extra_args.
Alternatively I could assert that datacarrier and maxmempool is set in extra_args?
I think documentation is enough for now, leaving as is.
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1556951038)
We can't restart the node after filling the memopool without resetting the floating minfee.
I could pull in the initial restart if we knew people didn't want any other extra_args.
Alternatively I could assert that datacarrier and maxmempool is set in extra_args?
I think documentation is enough for now, leaving as is.
π¬ Ashflower13 commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-2044199439)
Ok help need guidance Sent from my iPhoneOn Apr 8, 2024, at 18:17, tdb3 ***@***.***> wrote:ο»Ώ
@tdb3 commented on this pull request.
Great work catching the bug and making testing more robust.
ACK for d113709.
One inline recommendation to cover an additional related edge case.
In test/functional/rpc_createmultisig.py:
> @@ -118,6 +119,16 @@ def run_test(self):
# Check that bech32m is currently not allowed
assert_raises_rpc_error(-5, "createmultisig cannot create bech3
...
(https://github.com/bitcoin/bitcoin/pull/28312#issuecomment-2044199439)
Ok help need guidance Sent from my iPhoneOn Apr 8, 2024, at 18:17, tdb3 ***@***.***> wrote:ο»Ώ
@tdb3 commented on this pull request.
Great work catching the bug and making testing more robust.
ACK for d113709.
One inline recommendation to cover an additional related edge case.
In test/functional/rpc_createmultisig.py:
> @@ -118,6 +119,16 @@ def run_test(self):
# Check that bech32m is currently not allowed
assert_raises_rpc_error(-5, "createmultisig cannot create bech3
...
π¬ naiyoma commented on pull request "net: update comment for service bit support info for seed.bitcoin.sipa.be":
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2044216151)
@Sjors and @laanwj Thank you for the review. I'll look into how https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/check-dnsseeds.py can be modified . I'm pretty new to this codebase, and I was trying to figure out who supports which service bits.
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2044216151)
@Sjors and @laanwj Thank you for the review. I'll look into how https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/check-dnsseeds.py can be modified . I'm pretty new to this codebase, and I was trying to figure out who supports which service bits.
π naiyoma converted_to_draft a pull request: "net: update comment for service bit support info for seed.bitcoin.sipa.be"
(https://github.com/bitcoin/bitcoin/pull/29809)
This PR updates the comment regarding the supported service bits for `seed.bitcoin.sipa.be` based on this reference: https://github.com/sipa/bitcoin-seeder/blob/ff482e465ff84ea6fa276d858ccb7ef32e3355d3/main.cpp#L208
(https://github.com/bitcoin/bitcoin/pull/29809)
This PR updates the comment regarding the supported service bits for `seed.bitcoin.sipa.be` based on this reference: https://github.com/sipa/bitcoin-seeder/blob/ff482e465ff84ea6fa276d858ccb7ef32e3355d3/main.cpp#L208
β
brunoerg closed a pull request: "addrman: improve performance of `GetAddr` when specifying network"
(https://github.com/bitcoin/bitcoin/pull/29578)
(https://github.com/bitcoin/bitcoin/pull/29578)
π¬ brunoerg commented on pull request "addrman: improve performance of `GetAddr` when specifying network":
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2044281593)
Closing for now.
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2044281593)
Closing for now.
π fanquake merged a pull request: "depends: add new LLVM debug macro"
(https://github.com/bitcoin/bitcoin/pull/29781)
(https://github.com/bitcoin/bitcoin/pull/29781)
π fanquake opened a pull request: "Revert "ci: Temporarily disable bpfcc-tools""
(https://github.com/bitcoin/bitcoin/pull/29832)
This reverts commit fac012c7262f036e9b6f5800e57dcd63870a871c.
Closes #29804.
(https://github.com/bitcoin/bitcoin/pull/29832)
This reverts commit fac012c7262f036e9b6f5800e57dcd63870a871c.
Closes #29804.
π¬ laanwj commented on pull request "refactor: Optimize IsSpace function for common non-whitespace characters":
(https://github.com/bitcoin/bitcoin/pull/29602#issuecomment-2044334498)
Is this really a bottleneck? A benchmark would be useful to see if this isn't something the compiler already does.
ACK on the test.
(https://github.com/bitcoin/bitcoin/pull/29602#issuecomment-2044334498)
Is this really a bottleneck? A benchmark would be useful to see if this isn't something the compiler already does.
ACK on the test.